diff --git a/DEPS b/DEPS index a80a71d..7bbe07b 100644 --- a/DEPS +++ b/DEPS
@@ -40,11 +40,11 @@ # Three lines of non-changing comments so that # the commit queue can handle CLs rolling Skia # and whatever else without interference from each other. - 'skia_revision': '8b4b56ac2927bbb31c634f840916f4ce948d2f53', + 'skia_revision': 'b048e81c5c27fe6c6134eaf9ab96594e2eee0d1d', # Three lines of non-changing comments so that # the commit queue can handle CLs rolling V8 # and whatever else without interference from each other. - 'v8_revision': 'cae2c86ef45ca9314984d93da0512ca4a8190913', + 'v8_revision': 'e01db2c9ecb62bbdedb9c74c7e282bbaa8131eb3', # Three lines of non-changing comments so that # the commit queue can handle CLs rolling swarming_client # and whatever else without interference from each other.
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/ChromeFeatureList.java b/chrome/android/java/src/org/chromium/chrome/browser/ChromeFeatureList.java index 87e9ee2..1410b70 100644 --- a/chrome/android/java/src/org/chromium/chrome/browser/ChromeFeatureList.java +++ b/chrome/android/java/src/org/chromium/chrome/browser/ChromeFeatureList.java
@@ -66,6 +66,7 @@ public static final String NTP_SNIPPETS_SAVE_TO_OFFLINE = "NTPSaveToOffline"; public static final String NTP_SNIPPETS_OFFLINE_BADGE = "NTPOfflineBadge"; public static final String NTP_SUGGESTIONS_SECTION_DISMISSAL = "NTPSuggestionsSectionDismissal"; + public static final String SERVICE_WORKER_PAYMENT_APPS = "ServiceWorkerPaymentApps"; public static final String TAB_REPARENTING = "TabReparenting"; public static final String VR_SHELL = "VrShell"; public static final String WEB_PAYMENTS = "WebPayments";
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java b/chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java index 06bf964..69116d157 100644 --- a/chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java +++ b/chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java
@@ -7,6 +7,7 @@ import android.content.Context; import org.chromium.base.VisibleForTesting; +import org.chromium.chrome.browser.ChromeFeatureList; import org.chromium.content_public.browser.WebContents; import java.util.ArrayList; @@ -23,6 +24,12 @@ private static PaymentAppFactoryAddition sAdditionalFactory; /** + * Native bridge that can be used to retrieve information about installed service worker + * payment apps. + */ + private static ServiceWorkerPaymentAppBridge sServiceWorkerPaymentAppBridge; + + /** * The interface for additional payment app factories. */ public interface PaymentAppFactoryAddition { @@ -46,6 +53,15 @@ } /** + * Set a custom native bridge for service worker payment apps for testing. + */ + @VisibleForTesting + public static void setServiceWorkerPaymentAppBridgeForTest( + ServiceWorkerPaymentAppBridge bridge) { + sServiceWorkerPaymentAppBridge = bridge; + } + + /** * Builds instances of payment apps. * * @param context The context. @@ -54,10 +70,36 @@ public static List<PaymentApp> create(Context context, WebContents webContents) { List<PaymentApp> result = new ArrayList<>(2); result.add(new AutofillPaymentApp(context, webContents)); + + result.addAll(createServiceWorkerPaymentApps()); + if (sAdditionalFactory != null) { result.addAll( sAdditionalFactory.create(context, webContents)); } return result; } + + /** + * Build a ServiceWorkerPaymentApp instance for each installed service + * worker based payment app. + */ + private static List<ServiceWorkerPaymentApp> createServiceWorkerPaymentApps() { + if (sServiceWorkerPaymentAppBridge == null) { + if (ChromeFeatureList.isEnabled(ChromeFeatureList.SERVICE_WORKER_PAYMENT_APPS)) { + sServiceWorkerPaymentAppBridge = new ServiceWorkerPaymentAppBridge(); + } else { + return new ArrayList<>(); + } + } + + List<ServiceWorkerPaymentApp> result = new ArrayList<>(); + + for (ServiceWorkerPaymentAppBridge.Manifest manifest : + sServiceWorkerPaymentAppBridge.getAllAppManifests()) { + result.add(new ServiceWorkerPaymentApp(manifest)); + } + + return result; + } }
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java b/chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java index 309c961..c1cefc3 100644 --- a/chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java +++ b/chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
@@ -1121,7 +1121,8 @@ if (instruments != null) { for (int i = 0; i < instruments.size(); i++) { PaymentInstrument instrument = instruments.get(i); - Set<String> instrumentMethodNames = instrument.getInstrumentMethodNames(); + Set<String> instrumentMethodNames = + new HashSet<>(instrument.getInstrumentMethodNames()); instrumentMethodNames.retainAll(mMethodData.keySet()); if (!instrumentMethodNames.isEmpty()) { addPendingInstrument(instrument);
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentApp.java b/chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentApp.java new file mode 100644 index 0000000..84e3bfa --- /dev/null +++ b/chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentApp.java
@@ -0,0 +1,80 @@ +// Copyright 2016 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +package org.chromium.chrome.browser.payments; + +import android.os.Handler; + +import org.chromium.payments.mojom.PaymentMethodData; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; + +/** + * This app class represents a service worker based payment app. + * + * Such apps are implemented as service workers according to the Payment + * App API specification. + * + * @see https://w3c.github.io/webpayments-payment-apps-api/ + */ +public class ServiceWorkerPaymentApp implements PaymentApp { + private final ServiceWorkerPaymentAppBridge.Manifest mManifest; + private final Set<String> mMethodNames; + + /** + * Build a service worker payment app instance based on an installed manifest. + * + * @see https://w3c.github.io/webpayments-payment-apps-api/#payment-app-manifest + * + * @param manifest A manifest that describes this payment app. + */ + public ServiceWorkerPaymentApp(ServiceWorkerPaymentAppBridge.Manifest manifest) { + mManifest = manifest; + + mMethodNames = new HashSet<>(); + for (ServiceWorkerPaymentAppBridge.Option option : manifest.options) { + mMethodNames.addAll(option.enabledMethods); + } + } + + @Override + public void getInstruments( + Map<String, PaymentMethodData> unusedMethodData, + final InstrumentsCallback callback) { + final List<PaymentInstrument> instruments = + new ArrayList<PaymentInstrument>(); + + for (ServiceWorkerPaymentAppBridge.Option option : mManifest.options) { + instruments.add(new ServiceWorkerPaymentInstrument(mManifest.scopeUrl, option)); + } + + new Handler().post(new Runnable() { + @Override + public void run() { + callback.onInstrumentsReady(ServiceWorkerPaymentApp.this, instruments); + } + }); + } + + @Override + public Set<String> getAppMethodNames() { + return Collections.unmodifiableSet(mMethodNames); + } + + @Override + public boolean supportsMethodsAndData(Map<String, PaymentMethodData> methodsAndData) { + // TODO(tommyt): crbug.com/669876. Implement this for Service Worker Payment Apps. + return true; + } + + @Override + public String getAppIdentifier() { + return "Chrome_Service_Worker_Payment_App"; + } +}
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentAppBridge.java b/chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentAppBridge.java new file mode 100644 index 0000000..ac430d6 --- /dev/null +++ b/chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentAppBridge.java
@@ -0,0 +1,61 @@ +// Copyright 2016 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +package org.chromium.chrome.browser.payments; + +import android.graphics.drawable.Drawable; + +import org.chromium.base.annotations.SuppressFBWarnings; + +import java.util.ArrayList; +import java.util.List; + +/** + * Native bridge for interacting with service worker based payment apps. + */ +// TODO(tommyt): crbug.com/669876. Remove these suppressions when we actually +// start using all of the functionality in this class. +@SuppressFBWarnings({ + "UWF_UNWRITTEN_PUBLIC_OR_PROTECTED_FIELD", + "UUF_UNUSED_PUBLIC_OR_PROTECTED_FIELD"}) +public class ServiceWorkerPaymentAppBridge { + /** + * This class represents a payment app manifest as defined in the Payment + * App API specification. + * + * @see https://w3c.github.io/webpayments-payment-apps-api/#payment-app-manifest + */ + public static class Manifest { + /** + * The scope url of the service worker. + * + * This can be used to identify a service worker based payment app. + */ + public String scopeUrl; + public String label; + public Drawable icon; + public List<Option> options = new ArrayList<>(); + } + + /** + * This class represents a payment option as defined in the Payment App API + * specification. + * + * @see https://w3c.github.io/webpayments-payment-apps-api/#payment-app-options + */ + public static class Option { + public String id; + public String label; + public Drawable icon; + public List<String> enabledMethods = new ArrayList<>(); + } + + /** + * Get a list of all the installed app manifests. + */ + public List<Manifest> getAllAppManifests() { + // TODO(tommyt): crbug.com/669876. Implement this function. + return new ArrayList<Manifest>(); + } +}
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentInstrument.java b/chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentInstrument.java new file mode 100644 index 0000000..5dd4af9d --- /dev/null +++ b/chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentInstrument.java
@@ -0,0 +1,60 @@ +// Copyright 2016 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +package org.chromium.chrome.browser.payments; + +import org.chromium.payments.mojom.PaymentItem; +import org.chromium.payments.mojom.PaymentMethodData; + +import java.util.Collections; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; + +/** + * This instrument class represents a single payment option for a service + * worker based payment app. + * + * @see org.chromium.chrome.browser.payments.ServiceWorkerPaymentApp + * + * @see https://w3c.github.io/webpayments-payment-apps-api/ + */ +public class ServiceWorkerPaymentInstrument extends PaymentInstrument { + private final ServiceWorkerPaymentAppBridge.Option mOption; + private final Set<String> mMethodNames; + + /** + * Build a service worker based payment instrument based on a single payment option + * of an installed payment app. + * + * @see https://w3c.github.io/webpayments-payment-apps-api/#payment-app-options + * + * @param scopeUrl The scope url of the corresponding service worker payment app. + * @param option A payment app option from the payment app. + */ + public ServiceWorkerPaymentInstrument(String scopeUrl, + ServiceWorkerPaymentAppBridge.Option option) { + super(scopeUrl + "#" + option.id, option.label, null /* icon */, option.icon); + mOption = option; + + mMethodNames = new HashSet<String>(option.enabledMethods); + } + + @Override + public Set<String> getInstrumentMethodNames() { + return Collections.unmodifiableSet(mMethodNames); + } + + @Override + public void invokePaymentApp(String merchantName, String origin, PaymentItem total, + List<PaymentItem> cart, Map<String, PaymentMethodData> methodData, + InstrumentDetailsCallback callback) { + // TODO(tommyt): crbug.com/669876. Implement this for use with Service Worker Payment Apps. + callback.onInstrumentDetailsError(); + } + + @Override + public void dismissInstrument() {} +}
diff --git a/chrome/android/java_sources.gni b/chrome/android/java_sources.gni index 354c898..f32de15e 100644 --- a/chrome/android/java_sources.gni +++ b/chrome/android/java_sources.gni
@@ -686,6 +686,9 @@ "java/src/org/chromium/chrome/browser/payments/PaymentRequestMetrics.java", "java/src/org/chromium/chrome/browser/payments/PaymentResponseHelper.java", "java/src/org/chromium/chrome/browser/payments/PaymentValidator.java", + "java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentApp.java", + "java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentAppBridge.java", + "java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentInstrument.java", "java/src/org/chromium/chrome/browser/payments/ShippingStrings.java", "java/src/org/chromium/chrome/browser/payments/ui/BillingAddressAdapter.java", "java/src/org/chromium/chrome/browser/payments/ui/Completable.java", @@ -1350,6 +1353,7 @@ "javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPhoneAndFreeShippingTest.java", "javatests/src/org/chromium/chrome/browser/payments/PaymentRequestRemoveBillingAddressTest.java", "javatests/src/org/chromium/chrome/browser/payments/PaymentRequestServerCardTest.java", + "javatests/src/org/chromium/chrome/browser/payments/PaymentRequestServiceWorkerPaymentAppTest.java", "javatests/src/org/chromium/chrome/browser/payments/PaymentRequestShippingAddressTest.java", "javatests/src/org/chromium/chrome/browser/payments/PaymentRequestShowTwiceTest.java", "javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTabTest.java",
diff --git a/chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestServiceWorkerPaymentAppTest.java b/chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestServiceWorkerPaymentAppTest.java new file mode 100644 index 0000000..26f9cb5 --- /dev/null +++ b/chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestServiceWorkerPaymentAppTest.java
@@ -0,0 +1,107 @@ +// Copyright 2016 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +package org.chromium.chrome.browser.payments; + +import android.test.suitebuilder.annotation.MediumTest; + +import org.chromium.base.test.util.Feature; + +import java.util.Arrays; +import java.util.List; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.TimeoutException; + +/** + * A payment integration test for service worker based payment apps. + */ +public class PaymentRequestServiceWorkerPaymentAppTest extends PaymentRequestTestBase { + /** Flag for installing a service worker payment app without any payment options. */ + private static final int NO_OPTIONS = 0; + + /** Flag for installing a service worker payment app with one payment option. */ + private static final int ONE_OPTION = 1; + + /** Flag for installing a service worker payment app with two options. */ + private static final int TWO_OPTIONS = 2; + + public PaymentRequestServiceWorkerPaymentAppTest() { + super("payment_request_bobpay_test.html"); + } + + /** + * Installs a service worker based payment app for testing. + * + * @param optionPresence Whether the manifest has any payment options. Either NO_OPTIONS + * ONE_OPTION or TWO_OPTIONS + */ + private void installServiceWorkerPaymentApp(final int instrumentPresence) { + PaymentAppFactory.setServiceWorkerPaymentAppBridgeForTest( + new ServiceWorkerPaymentAppBridge() { + @Override + public List<Manifest> getAllAppManifests() { + ServiceWorkerPaymentAppBridge.Manifest testManifest = + new ServiceWorkerPaymentAppBridge.Manifest(); + testManifest.scopeUrl = "https://bobpay.com/app"; + testManifest.label = "BobPay"; + + if (instrumentPresence != NO_OPTIONS) { + ServiceWorkerPaymentAppBridge.Option testOption = + new ServiceWorkerPaymentAppBridge.Option(); + testOption.id = "new"; + testOption.label = "Create BobPay account"; + testOption.enabledMethods = + Arrays.asList("https://bobpay.com", "basic-card"); + testManifest.options.add(testOption); + } + + if (instrumentPresence == TWO_OPTIONS) { + ServiceWorkerPaymentAppBridge.Option testOption = + new ServiceWorkerPaymentAppBridge.Option(); + testOption.id = "existing"; + testOption.label = "Existing BobPay account"; + testOption.enabledMethods = + Arrays.asList("https://bobpay.com", "basic-card"); + testManifest.options.add(testOption); + } + + return Arrays.asList(testManifest); + } + }); + } + + @Override + public void onMainActivityStarted() throws InterruptedException, ExecutionException, + TimeoutException {} + + @MediumTest + @Feature({"Payments"}) + public void testNoOptions() throws InterruptedException, ExecutionException, + TimeoutException { + installServiceWorkerPaymentApp(NO_OPTIONS); + openPageAndClickBuyAndWait(mShowFailed); + expectResultContains( + new String[]{"show() rejected", "The payment method is not supported"}); + } + + @MediumTest + @Feature({"Payments"}) + public void testOneOption() throws InterruptedException, ExecutionException, + TimeoutException { + installServiceWorkerPaymentApp(ONE_OPTION); + triggerUIAndWait(mReadyForInput); + // TODO(tommyt): crbug.com/669876. Expand this test as we implement more + // service worker based payment app functionality. + } + + @MediumTest + @Feature({"Payments"}) + public void testTwoOptions() throws InterruptedException, ExecutionException, + TimeoutException { + installServiceWorkerPaymentApp(TWO_OPTIONS); + triggerUIAndWait(mReadyForInput); + // TODO(tommyt): crbug.com/669876. Expand this test as we implement more + // service worker based payment app functionality. + } +}
diff --git a/chrome/browser/android/chrome_feature_list.cc b/chrome/browser/android/chrome_feature_list.cc index 2476804..e31e369 100644 --- a/chrome/browser/android/chrome_feature_list.cc +++ b/chrome/browser/android/chrome_feature_list.cc
@@ -34,6 +34,7 @@ &autofill::kAutofillScanCardholderName, &features::kConsistentOmniboxGeolocation, &features::kCredentialManagementAPI, + &features::kServiceWorkerPaymentApps, &features::kSimplifiedFullscreenUI, &features::kVrShell, &features::kWebPayments,
diff --git a/chrome/browser/autofill/OWNERS b/chrome/browser/autofill/OWNERS index c6421d3..4de57ac 100644 --- a/chrome/browser/autofill/OWNERS +++ b/chrome/browser/autofill/OWNERS
@@ -1,4 +1,3 @@ estade@chromium.org -isherman@chromium.org mathp@chromium.org vabr@chromium.org
diff --git a/chrome/browser/content_settings/host_content_settings_map_unittest.cc b/chrome/browser/content_settings/host_content_settings_map_unittest.cc index 3fa66be..f779dd7b7 100644 --- a/chrome/browser/content_settings/host_content_settings_map_unittest.cc +++ b/chrome/browser/content_settings/host_content_settings_map_unittest.cc
@@ -1801,3 +1801,19 @@ EXPECT_EQ(ContentSettingsPattern::FromURLNoWildcard(url1), host_settings[0].primary_pattern); } + +TEST_F(HostContentSettingsMapTest, CanSetNarrowestSetting) { + TestingProfile profile; + const auto* map = HostContentSettingsMapFactory::GetForProfile(&profile); + + GURL valid_url("http://google.com"); + EXPECT_TRUE(map->CanSetNarrowestContentSetting( + valid_url, valid_url, + CONTENT_SETTINGS_TYPE_POPUPS)); + + GURL invalid_url("about:blank"); + EXPECT_FALSE(map->CanSetNarrowestContentSetting( + invalid_url, invalid_url, + CONTENT_SETTINGS_TYPE_POPUPS)); +} +
diff --git a/chrome/browser/resources/settings/site_settings/site_details.html b/chrome/browser/resources/settings/site_settings/site_details.html index abaff2edb..325c064 100644 --- a/chrome/browser/resources/settings/site_settings/site_details.html +++ b/chrome/browser/resources/settings/site_settings/site_details.html
@@ -37,9 +37,9 @@ <div class="settings-box first"> <div class="favicon-image" - style$="[[computeSiteIcon(site.originForDisplay)]]"> + style$="[[computeSiteIcon(site.origin)]]"> </div> - <div class="middle">[[site.originForDisplay]]</div> + <div class="middle">[[site.displayName]]</div> </div> <template is="dom-if" if="[[storedData_]]"> <div id="usage">
diff --git a/chrome/browser/resources/settings/site_settings/site_list.html b/chrome/browser/resources/settings/site_settings/site_list.html index d3e83dc0..99f1a761b 100644 --- a/chrome/browser/resources/settings/site_settings/site_list.html +++ b/chrome/browser/resources/settings/site_settings/site_list.html
@@ -66,10 +66,10 @@ <div class="start layout horizontal center" on-tap="onOriginTap_" actionable$="[[enableSiteSettings_]]"> <div class="favicon-image" - style$="[[computeSiteIcon(item.originForDisplay)]]"> + style$="[[computeSiteIcon(item.origin)]]"> </div> <div class="middle"> - <div class="selectable">[[item.originForDisplay]]</div> + <div class="selectable">[[item.displayName]]</div> <!-- This div must not contain extra whitespace. --> <div class="selectable secondary"
diff --git a/chrome/browser/resources/settings/site_settings/site_list.js b/chrome/browser/resources/settings/site_settings/site_list.js index 4847765..d121b5bed 100644 --- a/chrome/browser/resources/settings/site_settings/site_list.js +++ b/chrome/browser/resources/settings/site_settings/site_list.js
@@ -353,14 +353,14 @@ var siteException = this.expandSiteException(sites[i]); // The All Sites category can contain duplicates (from other categories). - if (this.allSites && siteException.originForDisplay == lastOrigin && - siteException.embeddingOriginForDisplay == lastEmbeddingOrigin) { + if (this.allSites && siteException.origin == lastOrigin && + siteException.embeddingOrigin == lastEmbeddingOrigin) { continue; } results.push(siteException); - lastOrigin = siteException.originForDisplay; - lastEmbeddingOrigin = siteException.embeddingOriginForDisplay; + lastOrigin = siteException.origin; + lastEmbeddingOrigin = siteException.embeddingOrigin; } return results; }, @@ -458,14 +458,14 @@ * @return {string} The site description. */ computeSiteDescription_: function(item) { - if (item.incognito && item.embeddingOriginForDisplay.length > 0) { + if (item.incognito && item.embeddingDisplayName.length > 0) { return loadTimeData.getStringF('embeddedIncognitoSite', - item.embeddingOriginForDisplay); + item.embeddingDisplayName); } if (item.incognito) return loadTimeData.getString('incognitoSite'); - return item.embeddingOriginForDisplay; + return item.embeddingDisplayName; }, /**
diff --git a/chrome/browser/resources/settings/site_settings/site_settings_behavior.js b/chrome/browser/resources/settings/site_settings/site_settings_behavior.js index 9aefbb2..2a38a6a 100644 --- a/chrome/browser/resources/settings/site_settings/site_settings_behavior.js +++ b/chrome/browser/resources/settings/site_settings/site_settings_behavior.js
@@ -289,23 +289,18 @@ */ expandSiteException: function(exception) { var origin = exception.origin; - // TODO(dschuyler): If orginForDisplay becomes different from origin in the - // site settings, that filtering would happen here. If that doesn't happen - // then originForDisplay should be removed (it's redundant with origin). - // e.g. var originForDisplay = someFilter(origin); - var embeddingOrigin = exception.embeddingOrigin; - var embeddingOriginForDisplay = ''; + var embeddingDisplayName = ''; if (origin != embeddingOrigin) { - embeddingOriginForDisplay = + embeddingDisplayName = this.getEmbedderString(embeddingOrigin, this.category); } return { origin: origin, - originForDisplay: origin, + displayName: exception.displayName, embeddingOrigin: embeddingOrigin, - embeddingOriginForDisplay: embeddingOriginForDisplay, + embeddingDisplayName: embeddingDisplayName, incognito: exception.incognito, setting: exception.setting, source: exception.source,
diff --git a/chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js b/chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js index 02b9ca22..9c934587 100644 --- a/chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js +++ b/chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js
@@ -9,10 +9,10 @@ /** * @typedef {{embeddingOrigin: string, - * embeddingOriginForDisplay: string, + * embeddingDisplayName: string, * incognito: boolean, * origin: string, - * originForDisplay: string, + * displayName: string, * setting: string, * source: string}} */
diff --git a/chrome/browser/resources/settings/site_settings/zoom_levels.html b/chrome/browser/resources/settings/site_settings/zoom_levels.html index c94a13a..c00584f 100644 --- a/chrome/browser/resources/settings/site_settings/zoom_levels.html +++ b/chrome/browser/resources/settings/site_settings/zoom_levels.html
@@ -23,10 +23,10 @@ rendered-item-count="{{renderedCount}}"> <div class="list-item"> <div class="favicon-image" - style$="[[computeSiteIcon(item.origin)]]"> + style$="[[computeSiteIcon(item.originForFavicon)]]"> </div> <div class="middle"> - <div>[[item.origin]]</div> + <div>[[item.displayName]]</div> </div> <div>[[item.zoom]]</div> <div>
diff --git a/chrome/browser/ui/autofill/OWNERS b/chrome/browser/ui/autofill/OWNERS index 8af22229..5d1596e 100644 --- a/chrome/browser/ui/autofill/OWNERS +++ b/chrome/browser/ui/autofill/OWNERS
@@ -1,2 +1,2 @@ estade@chromium.org -isherman@chromium.org +mathp@chromium.org
diff --git a/chrome/browser/ui/cocoa/autofill/OWNERS b/chrome/browser/ui/cocoa/autofill/OWNERS index c539e575..95d9d80 100644 --- a/chrome/browser/ui/cocoa/autofill/OWNERS +++ b/chrome/browser/ui/cocoa/autofill/OWNERS
@@ -1,2 +1,2 @@ groby@chromium.org -isherman@chromium.org +
diff --git a/chrome/browser/ui/content_settings/content_setting_bubble_model.cc b/chrome/browser/ui/content_settings/content_setting_bubble_model.cc index d03f35d1..d348884d 100644 --- a/chrome/browser/ui/content_settings/content_setting_bubble_model.cc +++ b/chrome/browser/ui/content_settings/content_setting_bubble_model.cc
@@ -401,7 +401,12 @@ setting == CONTENT_SETTING_BLOCK && PluginUtils::ShouldPreferHtmlOverPlugins( HostContentSettingsMapFactory::GetForProfile(profile())); - set_radio_group_enabled(setting_source == SETTING_SOURCE_USER && + + const auto* map = HostContentSettingsMapFactory::GetForProfile(profile()); + // Prevent creation of content settings for illegal urls like about:blank + bool is_valid = map->CanSetNarrowestContentSetting(url, url, content_type()); + + set_radio_group_enabled(is_valid && setting_source == SETTING_SOURCE_USER && !flash_hidden_from_plugin_list); selected_item_ = radio_group.default_item;
diff --git a/chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc b/chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc index 866ff283..678f307 100644 --- a/chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc +++ b/chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc
@@ -82,6 +82,8 @@ }; TEST_F(ContentSettingBubbleModelTest, ImageRadios) { + WebContentsTester::For(web_contents())-> + NavigateAndCommit(GURL("https://www.example.com")); TabSpecificContentSettings* content_settings = TabSpecificContentSettings::FromWebContents(web_contents()); content_settings->OnContentBlocked(CONTENT_SETTINGS_TYPE_IMAGES); @@ -99,6 +101,8 @@ } TEST_F(ContentSettingBubbleModelTest, Cookies) { + WebContentsTester::For(web_contents())-> + NavigateAndCommit(GURL("https://www.example.com")); TabSpecificContentSettings* content_settings = TabSpecificContentSettings::FromWebContents(web_contents()); content_settings->OnContentBlocked(CONTENT_SETTINGS_TYPE_COOKIES); @@ -713,6 +717,8 @@ } TEST_F(ContentSettingBubbleModelTest, Plugins) { + WebContentsTester::For(web_contents())-> + NavigateAndCommit(GURL("https://www.example.com")); TabSpecificContentSettings* content_settings = TabSpecificContentSettings::FromWebContents(web_contents()); const base::string16 plugin_name = base::ASCIIToUTF16("plugin_name"); @@ -737,6 +743,8 @@ } TEST_F(ContentSettingBubbleModelTest, PepperBroker) { + WebContentsTester::For(web_contents())-> + NavigateAndCommit(GURL("https://www.example.com")); TabSpecificContentSettings* content_settings = TabSpecificContentSettings::FromWebContents(web_contents()); content_settings->OnContentBlocked(CONTENT_SETTINGS_TYPE_PPAPI_BROKER); @@ -953,3 +961,37 @@ l10n_util::GetStringUTF8(IDS_FILTERED_DECEPTIVE_CONTENT_PROMPT_RELOAD)); EXPECT_EQ(0U, bubble_content.media_menus.size()); } + +TEST_F(ContentSettingBubbleModelTest, ValidUrl) { + WebContentsTester::For(web_contents())-> + NavigateAndCommit(GURL("https://www.example.com")); + + TabSpecificContentSettings* content_settings = + TabSpecificContentSettings::FromWebContents(web_contents()); + content_settings->OnContentBlocked(CONTENT_SETTINGS_TYPE_COOKIES); + + std::unique_ptr<ContentSettingBubbleModel> content_setting_bubble_model( + ContentSettingBubbleModel::CreateContentSettingBubbleModel( + NULL, web_contents(), profile(), CONTENT_SETTINGS_TYPE_COOKIES)); + const ContentSettingBubbleModel::BubbleContent& bubble_content = + content_setting_bubble_model->bubble_content(); + + EXPECT_TRUE(bubble_content.radio_group_enabled); +} + +TEST_F(ContentSettingBubbleModelTest, InvalidUrl) { + WebContentsTester::For(web_contents())-> + NavigateAndCommit(GURL("about:blank")); + + TabSpecificContentSettings* content_settings = + TabSpecificContentSettings::FromWebContents(web_contents()); + content_settings->OnContentBlocked(CONTENT_SETTINGS_TYPE_COOKIES); + + std::unique_ptr<ContentSettingBubbleModel> content_setting_bubble_model( + ContentSettingBubbleModel::CreateContentSettingBubbleModel( + NULL, web_contents(), profile(), CONTENT_SETTINGS_TYPE_COOKIES)); + const ContentSettingBubbleModel::BubbleContent& bubble_content = + content_setting_bubble_model->bubble_content(); + + EXPECT_FALSE(bubble_content.radio_group_enabled); +}
diff --git a/chrome/browser/ui/webui/options/content_settings_handler.cc b/chrome/browser/ui/webui/options/content_settings_handler.cc index 6e511d0..81f1017 100644 --- a/chrome/browser/ui/webui/options/content_settings_handler.cc +++ b/chrome/browser/ui/webui/options/content_settings_handler.cc
@@ -943,10 +943,12 @@ MediaSettingsInfo::ForOneType& settings = media_settings_->forType(type); HostContentSettingsMap* settings_map = HostContentSettingsMapFactory::GetForProfile(GetProfile()); - + const auto* extension_registry = + extensions::ExtensionRegistry::Get(GetProfile()); base::ListValue exceptions; - site_settings::GetExceptionsFromHostContentSettingsMap(settings_map, type, - web_ui(), /*incognito=*/false, /*filter=*/nullptr, &exceptions); + site_settings::GetExceptionsFromHostContentSettingsMap( + settings_map, type, extension_registry, web_ui(), /*incognito=*/false, + /*filter=*/nullptr, &exceptions); settings.exceptions.clear(); for (base::ListValue::const_iterator entry = exceptions.begin(); @@ -1090,8 +1092,11 @@ base::ListValue exceptions; HostContentSettingsMap* settings_map = HostContentSettingsMapFactory::GetForProfile(GetProfile()); - site_settings::GetExceptionsFromHostContentSettingsMap(settings_map, type, - web_ui(), /*incognito=*/false, /*filter=*/nullptr, &exceptions); + const auto* extension_registry = + extensions::ExtensionRegistry::Get(GetProfile()); + site_settings::GetExceptionsFromHostContentSettingsMap( + settings_map, type, extension_registry, web_ui(), /*incognito=*/false, + /*filter=*/nullptr, &exceptions); base::StringValue type_string( site_settings::ContentSettingsTypeToGroupName(type)); web_ui()->CallJavascriptFunctionUnsafe("ContentSettings.setExceptions", @@ -1116,9 +1121,12 @@ HostContentSettingsMapFactory::GetForProfile(GetOTRProfile()); if (!otr_settings_map) return; + const auto* extension_registry = + extensions::ExtensionRegistry::Get(GetOTRProfile()); base::ListValue exceptions; - site_settings::GetExceptionsFromHostContentSettingsMap(otr_settings_map, type, - web_ui(), /*incognito=*/true, /*filter=*/nullptr, &exceptions); + site_settings::GetExceptionsFromHostContentSettingsMap( + otr_settings_map, type, extension_registry, web_ui(), /*incognito=*/true, + /*filter=*/nullptr, &exceptions); base::StringValue type_string( site_settings::ContentSettingsTypeToGroupName(type)); web_ui()->CallJavascriptFunctionUnsafe("ContentSettings.setOTRExceptions",
diff --git a/chrome/browser/ui/webui/settings/site_settings_handler.cc b/chrome/browser/ui/webui/settings/site_settings_handler.cc index 71d5f14..efbd788 100644 --- a/chrome/browser/ui/webui/settings/site_settings_handler.cc +++ b/chrome/browser/ui/webui/settings/site_settings_handler.cc
@@ -23,6 +23,7 @@ #include "chrome/grit/generated_resources.h" #include "components/content_settings/core/browser/host_content_settings_map.h" #include "components/content_settings/core/common/content_settings_types.h" +#include "components/crx_file/id_util.h" #include "content/public/browser/browser_thread.h" #include "content/public/browser/notification_service.h" #include "content/public/browser/web_ui.h" @@ -418,18 +419,20 @@ HostContentSettingsMap* map = HostContentSettingsMapFactory::GetForProfile(profile_); + const auto* extension_registry = extensions::ExtensionRegistry::Get(profile_); AddExceptionsGrantedByHostedApps(profile_, APIPermissionFromGroupName(type), exceptions.get()); site_settings::GetExceptionsFromHostContentSettingsMap( - map, content_type, web_ui(), /*incognito=*/false, /*filter=*/nullptr, - exceptions.get()); + map, content_type, extension_registry, web_ui(), /*incognito=*/false, + /*filter=*/nullptr, exceptions.get()); if (profile_->HasOffTheRecordProfile()) { Profile* incognito = profile_->GetOffTheRecordProfile(); map = HostContentSettingsMapFactory::GetForProfile(incognito); + extension_registry = extensions::ExtensionRegistry::Get(incognito); site_settings::GetExceptionsFromHostContentSettingsMap( - map, content_type, web_ui(), /*incognito=*/true, /*filter=*/nullptr, - exceptions.get()); + map, content_type, extension_registry, web_ui(), /*incognito=*/true, + /*filter=*/nullptr, exceptions.get()); } ResolveJavascriptCallback(*callback_id, *exceptions.get()); @@ -546,16 +549,19 @@ HostContentSettingsMap* map = HostContentSettingsMapFactory::GetForProfile(profile_); + const auto* extension_registry = + extensions::ExtensionRegistry::Get(profile_); site_settings::GetExceptionsFromHostContentSettingsMap( - map, content_type, web_ui(), /*incognito=*/false, /*filter=*/&site, - exceptions.get()); + map, content_type, extension_registry, web_ui(), /*incognito=*/false, + /*filter=*/&site, exceptions.get()); if (profile_->HasOffTheRecordProfile()) { Profile* incognito = profile_->GetOffTheRecordProfile(); map = HostContentSettingsMapFactory::GetForProfile(incognito); + extension_registry = extensions::ExtensionRegistry::Get(incognito); site_settings::GetExceptionsFromHostContentSettingsMap( - map, content_type, web_ui(), /*incognito=*/true, /*filter=*/&site, - exceptions.get()); + map, content_type, extension_registry, web_ui(), /*incognito=*/true, + /*filter=*/&site, exceptions.get()); } } @@ -622,6 +628,8 @@ content::HostZoomMap::ZoomLevelVector zoom_levels( host_zoom_map->GetAllZoomLevels()); + const auto* extension_registry = extensions::ExtensionRegistry::Get(profile_); + // Sort ZoomLevelChanges by host and scheme // (a.com < http://a.com < https://a.com < b.com). std::sort(zoom_levels.begin(), zoom_levels.end(), @@ -629,20 +637,33 @@ const content::HostZoomMap::ZoomLevelChange& b) { return a.host == b.host ? a.scheme < b.scheme : a.host < b.host; }); - - for (content::HostZoomMap::ZoomLevelVector::const_iterator i = - zoom_levels.begin(); - i != zoom_levels.end(); - ++i) { + for (const auto& zoom_level : zoom_levels) { std::unique_ptr<base::DictionaryValue> exception(new base::DictionaryValue); - switch (i->mode) { + switch (zoom_level.mode) { case content::HostZoomMap::ZOOM_CHANGED_FOR_HOST: { - std::string host = i->host; + std::string host = zoom_level.host; if (host == content::kUnreachableWebDataURL) { host = l10n_util::GetStringUTF8(IDS_ZOOMLEVELS_CHROME_ERROR_PAGES_LABEL); } exception->SetString(site_settings::kOrigin, host); + + std::string display_name = host; + std::string origin_for_favicon = host; + // As an optimization, only check hosts that could be an extension. + if (crx_file::id_util::IdIsValid(host)) { + // Look up the host as an extension, if found then it is an extension. + const extensions::Extension* extension = + extension_registry->GetExtensionById( + host, extensions::ExtensionRegistry::EVERYTHING); + if (extension) { + origin_for_favicon = extension->url().spec(); + display_name = extension->name(); + } + } + exception->SetString(site_settings::kDisplayName, display_name); + exception->SetString(site_settings::kOriginForFavicon, + origin_for_favicon); break; } case content::HostZoomMap::ZOOM_CHANGED_FOR_SCHEME_AND_HOST: @@ -664,7 +685,7 @@ // Calculate the zoom percent from the factor. Round up to the nearest whole // number. int zoom_percent = static_cast<int>( - content::ZoomLevelToZoomFactor(i->zoom_level) * 100 + 0.5); + content::ZoomLevelToZoomFactor(zoom_level.zoom_level) * 100 + 0.5); exception->SetString(kZoom, base::FormatPercent(zoom_percent)); exception->SetString( site_settings::kSource, site_settings::kPreferencesSource);
diff --git a/chrome/browser/ui/webui/settings/site_settings_handler_unittest.cc b/chrome/browser/ui/webui/settings/site_settings_handler_unittest.cc index bad38882..8fb819a0 100644 --- a/chrome/browser/ui/webui/settings/site_settings_handler_unittest.cc +++ b/chrome/browser/ui/webui/settings/site_settings_handler_unittest.cc
@@ -261,8 +261,9 @@ ContentSettingsPattern pattern = ContentSettingsPattern::FromString("[*.]google.com"); std::unique_ptr<base::DictionaryValue> exception = - site_settings::GetExceptionForPage(pattern, pattern, - CONTENT_SETTING_BLOCK, "preference", false); + site_settings::GetExceptionForPage(pattern, pattern, pattern.ToString(), + CONTENT_SETTING_BLOCK, "preference", + false); std::string primary_pattern, secondary_pattern, type; bool incognito;
diff --git a/chrome/browser/ui/webui/site_settings_helper.cc b/chrome/browser/ui/webui/site_settings_helper.cc index 5daa96a..c1a6654 100644 --- a/chrome/browser/ui/webui/site_settings_helper.cc +++ b/chrome/browser/ui/webui/site_settings_helper.cc
@@ -15,6 +15,7 @@ #include "chrome/common/pref_names.h" #include "components/content_settings/core/browser/host_content_settings_map.h" #include "components/prefs/pref_service.h" +#include "extensions/browser/extension_registry.h" namespace site_settings { @@ -22,6 +23,8 @@ const char kAppId[] = "appId"; const char kSetting[] = "setting"; const char kOrigin[] = "origin"; +const char kDisplayName[] = "displayName"; +const char kOriginForFavicon[] = "originForFavicon"; const char kPolicyProviderId[] = "policy"; const char kSource[] = "source"; const char kIncognito[] = "incognito"; @@ -128,11 +131,13 @@ std::unique_ptr<base::DictionaryValue> GetExceptionForPage( const ContentSettingsPattern& pattern, const ContentSettingsPattern& secondary_pattern, + const std::string& display_name, const ContentSetting& setting, const std::string& provider_name, bool incognito) { base::DictionaryValue* exception = new base::DictionaryValue(); exception->SetString(kOrigin, pattern.ToString()); + exception->SetString(kDisplayName, display_name); exception->SetString(kEmbeddingOrigin, secondary_pattern == ContentSettingsPattern::Wildcard() ? std::string() : @@ -148,12 +153,31 @@ return base::WrapUnique(exception); } -void GetExceptionsFromHostContentSettingsMap(const HostContentSettingsMap* map, - ContentSettingsType type, - content::WebUI* web_ui, - bool incognito, - const std::string* filter, - base::ListValue* exceptions) { +std::string GetDisplayName( + const ContentSettingsPattern& pattern, + const extensions::ExtensionRegistry* extension_registry) { + if (extension_registry && + pattern.GetScheme() == ContentSettingsPattern::SCHEME_CHROMEEXTENSION) { + GURL url(pattern.ToString()); + // For the extension scheme, the pattern must be a valid URL. + DCHECK(url.is_valid()); + const extensions::Extension* extension = + extension_registry->GetExtensionById( + url.host(), extensions::ExtensionRegistry::EVERYTHING); + if (extension) + return extension->name(); + } + return pattern.ToString(); +} + +void GetExceptionsFromHostContentSettingsMap( + const HostContentSettingsMap* map, + ContentSettingsType type, + const extensions::ExtensionRegistry* extension_registry, + content::WebUI* web_ui, + bool incognito, + const std::string* filter, + base::ListValue* exceptions) { ContentSettingsForOneType entries; map->GetSettingsForOneType(type, std::string(), &entries); // Group settings by primary_pattern. @@ -194,6 +218,8 @@ ++i) { const ContentSettingsPattern& primary_pattern = i->first.first; const OnePatternSettings& one_settings = i->second; + const std::string display_name = + GetDisplayName(primary_pattern, extension_registry); // The "parent" entry either has an identical primary and secondary pattern, // or has a wildcard secondary. The two cases are indistinguishable in the @@ -212,8 +238,9 @@ parent == one_settings.end() ? CONTENT_SETTING_DEFAULT : parent->second; const ContentSettingsPattern& secondary_pattern = parent == one_settings.end() ? primary_pattern : parent->first; - this_provider_exceptions.push_back(GetExceptionForPage( - primary_pattern, secondary_pattern, parent_setting, source, incognito)); + this_provider_exceptions.push_back( + GetExceptionForPage(primary_pattern, secondary_pattern, display_name, + parent_setting, source, incognito)); // Add the "children" for any embedded settings. for (OnePatternSettings::const_iterator j = one_settings.begin(); @@ -223,8 +250,9 @@ continue; ContentSetting content_setting = j->second; - this_provider_exceptions.push_back(GetExceptionForPage( - primary_pattern, j->first, content_setting, source, incognito)); + this_provider_exceptions.push_back( + GetExceptionForPage(primary_pattern, j->first, display_name, + content_setting, source, incognito)); } } @@ -235,7 +263,8 @@ auto& policy_exceptions = all_provider_exceptions [HostContentSettingsMap::GetProviderTypeFromSource(kPolicyProviderId)]; DCHECK(policy_exceptions.empty()); - GetPolicyAllowedUrls(type, &policy_exceptions, web_ui, incognito); + GetPolicyAllowedUrls(type, &policy_exceptions, extension_registry, web_ui, + incognito); } for (auto& one_provider_exceptions : all_provider_exceptions) { @@ -247,6 +276,7 @@ void GetPolicyAllowedUrls( ContentSettingsType type, std::vector<std::unique_ptr<base::DictionaryValue>>* exceptions, + const extensions::ExtensionRegistry* extension_registry, content::WebUI* web_ui, bool incognito) { DCHECK(type == CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC || @@ -279,9 +309,10 @@ patterns.begin(), patterns.end(), std::greater<ContentSettingsPattern>()); for (const ContentSettingsPattern& pattern : patterns) { - exceptions->push_back(GetExceptionForPage(pattern, ContentSettingsPattern(), - CONTENT_SETTING_ALLOW, - kPolicyProviderId, incognito)); + std::string display_name = GetDisplayName(pattern, extension_registry); + exceptions->push_back(GetExceptionForPage( + pattern, ContentSettingsPattern(), display_name, CONTENT_SETTING_ALLOW, + kPolicyProviderId, incognito)); } }
diff --git a/chrome/browser/ui/webui/site_settings_helper.h b/chrome/browser/ui/webui/site_settings_helper.h index efda954..4bb415a 100644 --- a/chrome/browser/ui/webui/site_settings_helper.h +++ b/chrome/browser/ui/webui/site_settings_helper.h
@@ -26,6 +26,10 @@ class ListValue; } +namespace extensions { +class ExtensionRegistry; +} + namespace site_settings { // Maps from a secondary pattern to a setting. @@ -39,6 +43,8 @@ extern const char kSetting[]; extern const char kOrigin[]; +extern const char kDisplayName[]; +extern const char kOriginForFavicon[]; extern const char kPolicyProviderId[]; extern const char kSource[]; extern const char kIncognito[]; @@ -61,6 +67,7 @@ std::unique_ptr<base::DictionaryValue> GetExceptionForPage( const ContentSettingsPattern& pattern, const ContentSettingsPattern& secondary_pattern, + const std::string& display_name, const ContentSetting& setting, const std::string& provider_name, bool incognito); @@ -75,6 +82,7 @@ void GetExceptionsFromHostContentSettingsMap( const HostContentSettingsMap* map, ContentSettingsType type, + const extensions::ExtensionRegistry* extension_registry, content::WebUI* web_ui, bool incognito, const std::string* filter, @@ -85,6 +93,7 @@ void GetPolicyAllowedUrls( ContentSettingsType type, std::vector<std::unique_ptr<base::DictionaryValue>>* exceptions, + const extensions::ExtensionRegistry* extension_registry, content::WebUI* web_ui, bool incognito);
diff --git a/chrome/browser/ui/webui/site_settings_helper_unittest.cc b/chrome/browser/ui/webui/site_settings_helper_unittest.cc index 65c6879..7ea98848 100644 --- a/chrome/browser/ui/webui/site_settings_helper_unittest.cc +++ b/chrome/browser/ui/webui/site_settings_helper_unittest.cc
@@ -56,8 +56,8 @@ base::ListValue exceptions; // Check that the initial state of the map is empty. site_settings::GetExceptionsFromHostContentSettingsMap( - map, kContentType, nullptr /* web_ui */, false /* incognito */, - nullptr /* filter */, &exceptions); + map, kContentType, /*extension_registry=*/nullptr, /*web_ui=*/nullptr, + /*incognito=*/false, /*filter=*/nullptr, &exceptions); EXPECT_EQ(0u, exceptions.GetSize()); map->SetDefaultContentSetting(kContentType, CONTENT_SETTING_ALLOW); @@ -90,8 +90,8 @@ exceptions.Clear(); site_settings::GetExceptionsFromHostContentSettingsMap( - map, kContentType, nullptr /* web_ui */, false /* incognito */, - nullptr /* filter */, &exceptions); + map, kContentType, /*extension_registry=*/nullptr, /*web_ui=*/nullptr, + /*incognito=*/false, /*filter=*/nullptr, &exceptions); EXPECT_EQ(5u, exceptions.GetSize());
diff --git a/chrome/renderer/autofill/OWNERS b/chrome/renderer/autofill/OWNERS index 79f70d0..84f1716 100644 --- a/chrome/renderer/autofill/OWNERS +++ b/chrome/renderer/autofill/OWNERS
@@ -1,5 +1,4 @@ estade@chromium.org -isherman@chromium.org mathp@chromium.org vabr@chromium.org
diff --git a/chrome/test/data/webui/settings/site_details_tests.js b/chrome/test/data/webui/settings/site_details_tests.js index 8e59985..708772c 100644 --- a/chrome/test/data/webui/settings/site_details_tests.js +++ b/chrome/test/data/webui/settings/site_details_tests.js
@@ -136,7 +136,7 @@ var category = settings.ContentSettingsTypes.NOTIFICATIONS; var site = { origin: 'http://www.google.com', - originForDisplay: 'http://www.google.com', + displayName: 'http://www.google.com', embeddingOrigin: '', }; browserProxy.setPrefs(prefsEmpty); @@ -163,7 +163,7 @@ var category = settings.ContentSettingsTypes.NOTIFICATIONS; var site = { origin: 'https://foo-allow.com:443', - originForDisplay: 'https://foo-allow.com:443', + displayName: 'https://foo-allow.com:443', embeddingOrigin: '', }; @@ -201,7 +201,7 @@ browserProxy.setPrefs(prefs); testElement.site = { origin: 'https://foo-allow.com:443', - originForDisplay: 'https://foo-allow.com:443', + displayName: 'https://foo-allow.com:443', embeddingOrigin: '', };
diff --git a/chrome/test/data/webui/settings/site_list_tests.js b/chrome/test/data/webui/settings/site_list_tests.js index e3295a37..ce55862 100644 --- a/chrome/test/data/webui/settings/site_list_tests.js +++ b/chrome/test/data/webui/settings/site_list_tests.js
@@ -786,7 +786,7 @@ prefsMixedOriginAndPattern.exceptions. geolocation[0]. origin, - testElement.sites[0].originForDisplay); + testElement.sites[0].displayName); } assertEquals(undefined, testElement.selectedOrigin); @@ -800,7 +800,7 @@ prefsMixedOriginAndPattern.exceptions. geolocation[0]. origin, - testElement.selectedSite.originForDisplay); + testElement.selectedSite.displayName); } }); });
diff --git a/chrome/test/data/webui/settings/zoom_levels_tests.js b/chrome/test/data/webui/settings/zoom_levels_tests.js index b3e701c0..35d819eba 100644 --- a/chrome/test/data/webui/settings/zoom_levels_tests.js +++ b/chrome/test/data/webui/settings/zoom_levels_tests.js
@@ -25,12 +25,16 @@ var zoomList = [ { origin: 'http://www.google.com', + displayName: 'http://www.google.com', + originForFavicon: 'http://www.google.com', setting: '', source: '', zoom: '125%', }, { origin: 'http://www.chromium.org', + displayName: 'http://www.chromium.org', + originForFavicon: 'http://www.chromium.org', setting: '', source: '', zoom: '125%',
diff --git a/components/autofill/OWNERS b/components/autofill/OWNERS index be52fd5b..0ac929b 100644 --- a/components/autofill/OWNERS +++ b/components/autofill/OWNERS
@@ -1,5 +1,4 @@ estade@chromium.org -isherman@chromium.org mathp@chromium.org vabr@chromium.org
diff --git a/components/content_settings/core/browser/host_content_settings_map.cc b/components/content_settings/core/browser/host_content_settings_map.cc index a4a0770..0cb4f39 100644 --- a/components/content_settings/core/browser/host_content_settings_map.cc +++ b/components/content_settings/core/browser/host_content_settings_map.cc
@@ -161,6 +161,18 @@ return patterns; } +content_settings::PatternPair GetPatternsForContentSettingsType( + const GURL& primary_url, + const GURL& secondary_url, + ContentSettingsType type) { + const WebsiteSettingsInfo* website_settings_info = + content_settings::WebsiteSettingsRegistry::GetInstance()->Get(type); + DCHECK(website_settings_info); + content_settings::PatternPair patterns = GetPatternsFromScopingType( + website_settings_info->scoping_type(), primary_url, secondary_url); + return patterns; +} + } // namespace HostContentSettingsMap::HostContentSettingsMap(PrefService* prefs, @@ -349,11 +361,8 @@ ContentSettingsType content_type, const std::string& resource_identifier, std::unique_ptr<base::Value> value) { - const WebsiteSettingsInfo* info = - content_settings::WebsiteSettingsRegistry::GetInstance()->Get( - content_type); - content_settings::PatternPair patterns = GetPatternsFromScopingType( - info->scoping_type(), primary_url, secondary_url); + content_settings::PatternPair patterns = GetPatternsForContentSettingsType( + primary_url, secondary_url, content_type); ContentSettingsPattern primary_pattern = patterns.first; ContentSettingsPattern secondary_pattern = patterns.second; if (!primary_pattern.IsValid() || !secondary_pattern.IsValid()) @@ -385,11 +394,34 @@ NOTREACHED(); } +bool HostContentSettingsMap::CanSetNarrowestContentSetting( + const GURL& primary_url, + const GURL& secondary_url, + ContentSettingsType type) const { + content_settings::PatternPair patterns = + GetNarrowestPatterns(primary_url, secondary_url, type); + return patterns.first.IsValid() && patterns.second.IsValid(); +} + void HostContentSettingsMap::SetNarrowestContentSetting( const GURL& primary_url, const GURL& secondary_url, ContentSettingsType type, ContentSetting setting) { + content_settings::PatternPair patterns = + GetNarrowestPatterns(primary_url, secondary_url, type); + + if (!patterns.first.IsValid() || !patterns.second.IsValid()) + return; + + SetContentSettingCustomScope(patterns.first, patterns.second, type, + std::string(), setting); +} + +content_settings::PatternPair HostContentSettingsMap::GetNarrowestPatterns ( + const GURL& primary_url, + const GURL& secondary_url, + ContentSettingsType type) const { // Permission settings are specified via rules. There exists always at least // one rule for the default setting. Get the rule that currently defines // the permission for the given permission |type|. Then test whether the @@ -401,29 +433,23 @@ primary_url, secondary_url, type, std::string(), &info); DCHECK_EQ(content_settings::SETTING_SOURCE_USER, info.source); - const WebsiteSettingsInfo* website_settings_info = - content_settings::WebsiteSettingsRegistry::GetInstance()->Get(type); - content_settings::PatternPair patterns = GetPatternsFromScopingType( - website_settings_info->scoping_type(), primary_url, secondary_url); - - ContentSettingsPattern narrow_primary = patterns.first; - ContentSettingsPattern narrow_secondary = patterns.second; + content_settings::PatternPair patterns = GetPatternsForContentSettingsType( + primary_url, secondary_url, type); ContentSettingsPattern::Relation r1 = info.primary_pattern.Compare(patterns.first); if (r1 == ContentSettingsPattern::PREDECESSOR) { - narrow_primary = info.primary_pattern; + patterns.first = info.primary_pattern; } else if (r1 == ContentSettingsPattern::IDENTITY) { ContentSettingsPattern::Relation r2 = info.secondary_pattern.Compare(patterns.second); DCHECK(r2 != ContentSettingsPattern::DISJOINT_ORDER_POST && r2 != ContentSettingsPattern::DISJOINT_ORDER_PRE); if (r2 == ContentSettingsPattern::PREDECESSOR) - narrow_secondary = info.secondary_pattern; + patterns.second = info.secondary_pattern; } - SetContentSettingCustomScope(narrow_primary, narrow_secondary, type, - std::string(), setting); + return patterns; } void HostContentSettingsMap::SetContentSettingCustomScope( @@ -458,14 +484,9 @@ ContentSettingsType content_type, const std::string& resource_identifier, ContentSetting setting) { - const ContentSettingsInfo* info = - content_settings::ContentSettingsRegistry::GetInstance()->Get( - content_type); - DCHECK(info); + content_settings::PatternPair patterns = GetPatternsForContentSettingsType( + primary_url, secondary_url, content_type); - content_settings::PatternPair patterns = - GetPatternsFromScopingType(info->website_settings_info()->scoping_type(), - primary_url, secondary_url); ContentSettingsPattern primary_pattern = patterns.first; ContentSettingsPattern secondary_pattern = patterns.second; if (!primary_pattern.IsValid() || !secondary_pattern.IsValid())
diff --git a/components/content_settings/core/browser/host_content_settings_map.h b/components/content_settings/core/browser/host_content_settings_map.h index bf174b51..e970af8 100644 --- a/components/content_settings/core/browser/host_content_settings_map.h +++ b/components/content_settings/core/browser/host_content_settings_map.h
@@ -196,6 +196,13 @@ const std::string& resource_identifier, std::unique_ptr<base::Value> value); + // Check if a call to SetNarrowestContentSetting would succeed or if it would + // fail because of an invalid pattern. + bool CanSetNarrowestContentSetting( + const GURL& primary_url, + const GURL& secondary_url, + ContentSettingsType type) const; + // Sets the most specific rule that currently defines the setting for the // given content type. TODO(raymes): Remove this once all content settings // are scoped to origin scope. There is no scope more narrow than origin @@ -364,6 +371,11 @@ const std::string& resource_identifier, content_settings::SettingInfo* info) const; + content_settings::PatternPair GetNarrowestPatterns( + const GURL& primary_url, + const GURL& secondary_url, + ContentSettingsType type) const; + static std::unique_ptr<base::Value> GetContentSettingValueAndPatterns( const content_settings::ProviderInterface* provider, const GURL& primary_url,
diff --git a/components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc b/components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc index 72825564..63be8ad 100644 --- a/components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc +++ b/components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc
@@ -75,7 +75,7 @@ for (auto iter = bookmarks.begin(); iter != bookmarks.end(); ++iter) { base::Time last_visited; - if (GetLastVisitDateForNTPBookmark(*iter, consider_visits_from_desktop, + if (GetLastVisitDateForNTPBookmark(**iter, consider_visits_from_desktop, &last_visited) && most_recent_last_visited <= last_visited) { most_recent = iter; @@ -116,20 +116,20 @@ } } -bool GetLastVisitDateForNTPBookmark(const BookmarkNode* node, +bool GetLastVisitDateForNTPBookmark(const BookmarkNode& node, bool consider_visits_from_desktop, base::Time* out) { - if (!node || IsDismissedFromNTPForBookmark(node)) { + if (IsDismissedFromNTPForBookmark(node)) { return false; } bool got_mobile_date = - ExtractLastVisitDate(*node, kBookmarkLastVisitDateOnMobileKey, out); + ExtractLastVisitDate(node, kBookmarkLastVisitDateOnMobileKey, out); if (consider_visits_from_desktop) { // Consider the later visit from these two platform groups. base::Time last_visit_desktop; - if (ExtractLastVisitDate(*node, kBookmarkLastVisitDateOnDesktopKey, + if (ExtractLastVisitDate(node, kBookmarkLastVisitDateOnDesktopKey, &last_visit_desktop)) { if (!got_mobile_date) { *out = last_visit_desktop; @@ -151,14 +151,10 @@ } } -bool IsDismissedFromNTPForBookmark(const BookmarkNode* node) { - if (!node) { - return false; - } - +bool IsDismissedFromNTPForBookmark(const BookmarkNode& node) { std::string dismissed_from_ntp; bool result = - node->GetMetaInfo(kBookmarkDismissedFromNTP, &dismissed_from_ntp); + node.GetMetaInfo(kBookmarkDismissedFromNTP, &dismissed_from_ntp); DCHECK(!result || dismissed_from_ntp == "1"); return result; } @@ -211,7 +207,7 @@ // Extract the last visit of the node to use later for sorting. base::Time last_visit_time; if (!GetLastVisitDateForNTPBookmark( - *most_recent, consider_visits_from_desktop, &last_visit_time) || + **most_recent, consider_visits_from_desktop, &last_visit_time) || last_visit_time <= min_visit_time) { continue; } @@ -256,7 +252,7 @@ DCHECK(!bookmarks_for_url.empty()); for (const BookmarkNode* node : bookmarks_for_url) { - if (!IsDismissedFromNTPForBookmark(node)) { + if (!IsDismissedFromNTPForBookmark(*node)) { return true; } }
diff --git a/components/ntp_snippets/bookmarks/bookmark_last_visit_utils.h b/components/ntp_snippets/bookmarks/bookmark_last_visit_utils.h index cdd5cb69..b022e8e3 100644 --- a/components/ntp_snippets/bookmarks/bookmark_last_visit_utils.h +++ b/components/ntp_snippets/bookmarks/bookmark_last_visit_utils.h
@@ -35,7 +35,7 @@ // As visits, we primarily understand visits on Android (the visit when the // bookmark is created also counts). Visits on desktop platforms are considered // only if |consider_visits_from_desktop|. -bool GetLastVisitDateForNTPBookmark(const bookmarks::BookmarkNode* node, +bool GetLastVisitDateForNTPBookmark(const bookmarks::BookmarkNode& node, bool consider_visits_from_desktop, base::Time* out); @@ -44,7 +44,7 @@ const GURL& url); // Gets the dismissed flag for a given bookmark |node|. Defaults to false. -bool IsDismissedFromNTPForBookmark(const bookmarks::BookmarkNode* node); +bool IsDismissedFromNTPForBookmark(const bookmarks::BookmarkNode& node); // Removes the dismissed flag from all bookmarks (only for debugging). void MarkAllBookmarksUndismissed(bookmarks::BookmarkModel* bookmark_model);
diff --git a/components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc b/components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc index 8c80986..b27ba5d9 100644 --- a/components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc +++ b/components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc
@@ -176,7 +176,7 @@ std::vector<ContentSuggestion> suggestions; for (const BookmarkNode* bookmark : bookmarks) { - ConvertBookmark(bookmark, &suggestions); + ConvertBookmark(*bookmark, &suggestions); } callback.Run(std::move(suggestions)); } @@ -204,7 +204,7 @@ BookmarkModel* model, const BookmarkNode* node) { // Store the last visit date of the node that is about to change. - if (!GetLastVisitDateForNTPBookmark(node, + if (!GetLastVisitDateForNTPBookmark(*node, consider_bookmark_visits_from_desktop_, &node_to_change_last_visit_date_)) { node_to_change_last_visit_date_ = base::Time::UnixEpoch(); @@ -216,7 +216,7 @@ const BookmarkNode* node) { base::Time time; if (!GetLastVisitDateForNTPBookmark( - node, consider_bookmark_visits_from_desktop_, &time)) { + *node, consider_bookmark_visits_from_desktop_, &time)) { // Error in loading the last visit date after the change. This happens when // the bookmark just got dismissed. We must not update the suggestion in // such a case. @@ -241,7 +241,7 @@ const std::set<GURL>& no_longer_bookmarked) { base::Time time; if (GetLastVisitDateForNTPBookmark( - node, consider_bookmark_visits_from_desktop_, &time) && + *node, consider_bookmark_visits_from_desktop_, &time) && time < end_of_list_last_visit_date_) { // We know the node is too old to influence the list. return; @@ -256,7 +256,7 @@ const bookmarks::BookmarkNode* parent, int index) { base::Time time; - if (!GetLastVisitDateForNTPBookmark(parent->GetChild(index), + if (!GetLastVisitDateForNTPBookmark(*parent->GetChild(index), consider_bookmark_visits_from_desktop_, &time) || time < end_of_list_last_visit_date_) { @@ -269,7 +269,7 @@ } void BookmarkSuggestionsProvider::ConvertBookmark( - const BookmarkNode* bookmark, + const BookmarkNode& bookmark, std::vector<ContentSuggestion>* suggestions) { base::Time publish_date; if (!GetLastVisitDateForNTPBookmark( @@ -277,12 +277,12 @@ return; } - ContentSuggestion suggestion(provided_category_, bookmark->url().spec(), - bookmark->url()); - suggestion.set_title(bookmark->GetTitle()); + ContentSuggestion suggestion(provided_category_, bookmark.url().spec(), + bookmark.url()); + suggestion.set_title(bookmark.GetTitle()); suggestion.set_snippet_text(base::string16()); suggestion.set_publish_date(publish_date); - suggestion.set_publisher_name(base::UTF8ToUTF16(bookmark->url().host())); + suggestion.set_publisher_name(base::UTF8ToUTF16(bookmark.url().host())); suggestions->emplace_back(std::move(suggestion)); } @@ -299,7 +299,7 @@ std::vector<ContentSuggestion> suggestions; for (const BookmarkNode* bookmark : bookmarks) { - ConvertBookmark(bookmark, &suggestions); + ConvertBookmark(*bookmark, &suggestions); } if (suggestions.empty()) {
diff --git a/components/ntp_snippets/bookmarks/bookmark_suggestions_provider.h b/components/ntp_snippets/bookmarks/bookmark_suggestions_provider.h index fdbdbc97..91231ae 100644 --- a/components/ntp_snippets/bookmarks/bookmark_suggestions_provider.h +++ b/components/ntp_snippets/bookmarks/bookmark_suggestions_provider.h
@@ -87,7 +87,7 @@ bookmarks::BookmarkModel* model, const std::set<GURL>& removed_urls) override {} - void ConvertBookmark(const bookmarks::BookmarkNode* bookmark, + void ConvertBookmark(const bookmarks::BookmarkNode& bookmark, std::vector<ContentSuggestion>* suggestions); // The actual method to fetch bookmarks - follows each call to FetchBookmarks
diff --git a/components/ntp_snippets/remote/ntp_snippets_fetcher.cc b/components/ntp_snippets/remote/ntp_snippets_fetcher.cc index 0876b51..62236a8 100644 --- a/components/ntp_snippets/remote/ntp_snippets_fetcher.cc +++ b/components/ntp_snippets/remote/ntp_snippets_fetcher.cc
@@ -5,6 +5,7 @@ #include "components/ntp_snippets/remote/ntp_snippets_fetcher.h" #include <cstdlib> +#include <utility> #include "base/command_line.h" #include "base/files/file_path.h" @@ -33,7 +34,6 @@ #include "components/variations/variations_associated_data.h" #include "grit/components_strings.h" #include "net/base/load_flags.h" -#include "net/http/http_request_headers.h" #include "net/http/http_response_headers.h" #include "net/http/http_status_code.h" #include "net/url_request/url_fetcher.h" @@ -109,25 +109,6 @@ return "Unknown error"; } -bool IsFetchPreconditionFailed(NTPSnippetsFetcher::FetchResult result) { - switch (result) { - case NTPSnippetsFetcher::FetchResult::DEPRECATED_EMPTY_HOSTS: - case NTPSnippetsFetcher::FetchResult::OAUTH_TOKEN_ERROR: - case NTPSnippetsFetcher::FetchResult::INTERACTIVE_QUOTA_ERROR: - case NTPSnippetsFetcher::FetchResult::NON_INTERACTIVE_QUOTA_ERROR: - return true; - case NTPSnippetsFetcher::FetchResult::SUCCESS: - case NTPSnippetsFetcher::FetchResult::URL_REQUEST_STATUS_ERROR: - case NTPSnippetsFetcher::FetchResult::HTTP_ERROR: - case NTPSnippetsFetcher::FetchResult::JSON_PARSE_ERROR: - case NTPSnippetsFetcher::FetchResult::INVALID_SNIPPET_CONTENT_ERROR: - case NTPSnippetsFetcher::FetchResult::RESULT_MAX: - return false; - } - NOTREACHED(); - return true; -} - std::string GetFetchEndpoint() { std::string endpoint = variations::GetVariationParamValue( ntp_snippets::kStudyName, kContentSuggestionsBackend); @@ -137,7 +118,7 @@ bool IsBooleanParameterEnabled(const std::string& param_name, bool default_value) { std::string param_value = variations::GetVariationParamValueByFeature( - ntp_snippets::kArticleSuggestionsFeature, param_name); + ntp_snippets::kArticleSuggestionsFeature, param_name); if (param_value == kBooleanParameterEnabled) { return true; } @@ -151,14 +132,6 @@ return default_value; } -bool IsSendingTopLanguagesEnabled() { - return IsBooleanParameterEnabled(kSendTopLanguagesName, false); -} - -bool IsSendingUserClassEnabled() { - return IsBooleanParameterEnabled(kSendUserClassName, false); -} - bool UsesChromeContentSuggestionsAPI(const GURL& endpoint) { if (endpoint == kChromeReaderServer) { return false; @@ -262,8 +235,91 @@ return now_exploded.hour * 60 + now_minute; } +// The response from the backend might include suggestions from multiple +// categories. If only a single category was requested, this function filters +// all other categories out. +void FilterCategories(NTPSnippetsFetcher::FetchedCategoriesVector* categories, + base::Optional<Category> exclusive_category) { + if (!exclusive_category.has_value()) { + return; + } + Category exclusive = exclusive_category.value(); + auto category_it = std::find_if( + categories->begin(), categories->end(), + [&exclusive](const NTPSnippetsFetcher::FetchedCategory& c) -> bool { + return c.category == exclusive; + }); + if (category_it == categories->end()) { + categories->clear(); + return; + } + NTPSnippetsFetcher::FetchedCategory category = std::move(*category_it); + categories->clear(); + categories->push_back(std::move(category)); +} + } // namespace +// A single request to query snippets. +class NTPSnippetsFetcher::JsonRequest : public net::URLFetcherDelegate { + public: + JsonRequest(base::Optional<Category> exclusive_category, + base::TickClock* tick_clock, + const ParseJSONCallback& callback); + JsonRequest(JsonRequest&&); + ~JsonRequest() override; + + // A client can expect error_details only, if there was any error during the + // fetching or parsing. In successful cases, it will be an empty string. + using CompletedCallback = + base::OnceCallback<void(std::unique_ptr<base::Value> result, + FetchResult result_code, + const std::string& error_details)>; + + void Start(CompletedCallback callback); + + const base::Optional<Category>& exclusive_category() const { + return exclusive_category_; + } + + base::TimeDelta GetFetchDuration() const; + std::string GetResponseString() const; + + private: + friend class RequestBuilder; + + // URLFetcherDelegate implementation. + void OnURLFetchComplete(const net::URLFetcher* source) override; + + void ParseJsonResponse(); + void OnJsonParsed(std::unique_ptr<base::Value> result); + void OnJsonError(const std::string& error); + + // The fetcher for downloading the snippets. Only non-null if a fetch is + // currently ongoing. + std::unique_ptr<net::URLFetcher> url_fetcher_; + + // If set, only return results for this category. + base::Optional<Category> exclusive_category_; + + // Use the TickClock from the Fetcher to measure the fetch time. It will be + // used on creation and after the fetch returned. It has to be alive until the + // request is destroyed. + base::TickClock* tick_clock_; + base::TimeTicks creation_time_; + + // This callback is called to parse a json string. It contains callbacks for + // error and success cases. + ParseJSONCallback parse_json_callback_; + + // The callback to notify when URLFetcher finished and results are available. + CompletedCallback request_completed_callback_; + + base::WeakPtrFactory<JsonRequest> weak_ptr_factory_; + + DISALLOW_COPY_AND_ASSIGN(JsonRequest); +}; + CategoryInfo BuildArticleCategoryInfo( const base::Optional<base::string16>& title) { return CategoryInfo( @@ -303,7 +359,9 @@ NTPSnippetsFetcher::FetchedCategory::FetchedCategory(FetchedCategory&&) = default; + NTPSnippetsFetcher::FetchedCategory::~FetchedCategory() = default; + NTPSnippetsFetcher::FetchedCategory& NTPSnippetsFetcher::FetchedCategory:: operator=(FetchedCategory&&) = default; @@ -330,8 +388,8 @@ parse_json_callback_(parse_json_callback), fetch_url_(GetFetchEndpoint()), fetch_api_(UsesChromeContentSuggestionsAPI(fetch_url_) - ? CHROME_CONTENT_SUGGESTIONS_API - : CHROME_READER_API), + ? FetchAPI::CHROME_CONTENT_SUGGESTIONS_API + : FetchAPI::CHROME_READER_API), api_key_(api_key), tick_clock_(new base::DefaultTickClock()), user_classifier_(user_classifier), @@ -348,7 +406,6 @@ RequestThrottler::RequestType:: CONTENT_SUGGESTION_FETCHER_ACTIVE_SUGGESTIONS_CONSUMER), weak_ptr_factory_(this) { - // Parse the variation parameters and set the defaults if missing. std::string personalization = variations::GetVariationParamValue( ntp_snippets::kStudyName, kPersonalizationName); if (personalization == kPersonalizationNonPersonalString) { @@ -373,11 +430,11 @@ void NTPSnippetsFetcher::FetchSnippets(const Params& params, SnippetsAvailableCallback callback) { if (!DemandQuotaForRequest(params.interactive_request)) { - FetchFinished(OptionalFetchedCategories(), + FetchFinished(OptionalFetchedCategories(), std::move(callback), params.interactive_request ? FetchResult::INTERACTIVE_QUOTA_ERROR : FetchResult::NON_INTERACTIVE_QUOTA_ERROR, - /*extra_message=*/std::string()); + /*error_details=*/std::string()); return; } @@ -390,18 +447,26 @@ /*reduced_resolution=*/true)); } - params_ = params; - fetch_start_time_ = tick_clock_->NowTicks(); - snippets_available_callback_ = std::move(callback); + RequestBuilder builder; + builder.SetFetchAPI(fetch_api_) + .SetFetchAPI(fetch_api_) + .SetLanguageModel(language_model_) + .SetParams(params) + .SetParseJsonCallback(parse_json_callback_) + .SetPersonalization(personalization_) + .SetTickClock(tick_clock_.get()) + .SetUrlRequestContextGetter(url_request_context_getter_) + .SetUserClassifier(*user_classifier_); - bool use_authentication = UsesAuthentication(); - if (use_authentication && signin_manager_->IsAuthenticated()) { + if (NeedsAuthentication() && signin_manager_->IsAuthenticated()) { // Signed-in: get OAuth token --> fetch snippets. oauth_token_retried_ = false; + pending_requests_.emplace(std::move(builder), std::move(callback)); StartTokenRequest(); - } else if (use_authentication && signin_manager_->AuthInProgress()) { + } else if (NeedsAuthentication() && signin_manager_->AuthInProgress()) { // Currently signing in: wait for auth to finish (the refresh token) --> // get OAuth token --> fetch snippets. + pending_requests_.emplace(std::move(builder), std::move(callback)); if (!waiting_for_refresh_token_) { // Wait until we get a refresh token. waiting_for_refresh_token_ = true; @@ -409,24 +474,225 @@ } } else { // Not signed in: fetch snippets (without authentication). - FetchSnippetsNonAuthenticated(); + FetchSnippetsNonAuthenticated(std::move(builder), std::move(callback)); } } -NTPSnippetsFetcher::RequestBuilder::RequestBuilder() = default; +NTPSnippetsFetcher::JsonRequest::JsonRequest( + base::Optional<Category> exclusive_category, + base::TickClock* tick_clock, // Needed until destruction of the request. + const ParseJSONCallback& callback) + : exclusive_category_(exclusive_category), + tick_clock_(tick_clock), + parse_json_callback_(callback), + weak_ptr_factory_(this) { + creation_time_ = tick_clock_->NowTicks(); +} +NTPSnippetsFetcher::JsonRequest::~JsonRequest() { + LOG_IF(DFATAL, !request_completed_callback_.is_null()) + << "The CompletionCallback was never called!"; +} + +void NTPSnippetsFetcher::JsonRequest::Start(CompletedCallback callback) { + request_completed_callback_ = std::move(callback); + url_fetcher_->Start(); +} + +base::TimeDelta NTPSnippetsFetcher::JsonRequest::GetFetchDuration() const { + return tick_clock_->NowTicks() - creation_time_; +} + +std::string NTPSnippetsFetcher::JsonRequest::GetResponseString() const { + std::string response; + url_fetcher_->GetResponseAsString(&response); + return response; +} + +//////////////////////////////////////////////////////////////////////////////// +// URLFetcherDelegate overrides +void NTPSnippetsFetcher::JsonRequest::OnURLFetchComplete( + const net::URLFetcher* source) { + DCHECK_EQ(url_fetcher_.get(), source); + const URLRequestStatus& status = url_fetcher_->GetStatus(); + int response = url_fetcher_->GetResponseCode(); + UMA_HISTOGRAM_SPARSE_SLOWLY( + "NewTabPage.Snippets.FetchHttpResponseOrErrorCode", + status.is_success() ? response : status.error()); + + if (!status.is_success()) { + std::move(request_completed_callback_) + .Run(/*result=*/nullptr, FetchResult::URL_REQUEST_STATUS_ERROR, + /*error_details=*/base::StringPrintf(" %d", status.error())); + } else if (response != net::HTTP_OK) { + // TODO(jkrcal): https://crbug.com/609084 + // We need to deal with the edge case again where the auth + // token expires just before we send the request (in which case we need to + // fetch a new auth token). We should extract that into a common class + // instead of adding it to every single class that uses auth tokens. + std::move(request_completed_callback_) + .Run(/*result=*/nullptr, FetchResult::HTTP_ERROR, + /*error_details=*/base::StringPrintf(" %d", response)); + } else { + ParseJsonResponse(); + } +} + +void NTPSnippetsFetcher::JsonRequest::ParseJsonResponse() { + std::string json_string; + bool stores_result_to_string = + url_fetcher_->GetResponseAsString(&json_string); + DCHECK(stores_result_to_string); + + parse_json_callback_.Run( + json_string, + base::Bind(&JsonRequest::OnJsonParsed, weak_ptr_factory_.GetWeakPtr()), + base::Bind(&JsonRequest::OnJsonError, weak_ptr_factory_.GetWeakPtr())); +} + +void NTPSnippetsFetcher::JsonRequest::OnJsonParsed( + std::unique_ptr<base::Value> result) { + std::move(request_completed_callback_) + .Run(std::move(result), FetchResult::SUCCESS, + /*error_details=*/std::string()); +} + +void NTPSnippetsFetcher::JsonRequest::OnJsonError(const std::string& error) { + std::string json_string; + url_fetcher_->GetResponseAsString(&json_string); + LOG(WARNING) << "Received invalid JSON (" << error << "): " << json_string; + std::move(request_completed_callback_) + .Run(/*result=*/nullptr, FetchResult::JSON_PARSE_ERROR, + /*error_details=*/base::StringPrintf(" (error %s)", error.c_str())); +} + +NTPSnippetsFetcher::RequestBuilder::RequestBuilder() + : fetch_api_(CHROME_READER_API), + personalization_(Personalization::kBoth), + language_model_(nullptr) {} NTPSnippetsFetcher::RequestBuilder::RequestBuilder(RequestBuilder&&) = default; - NTPSnippetsFetcher::RequestBuilder::~RequestBuilder() = default; -std::string NTPSnippetsFetcher::RequestBuilder::BuildRequest() { +std::unique_ptr<NTPSnippetsFetcher::JsonRequest> +NTPSnippetsFetcher::RequestBuilder::Build() const { + DCHECK(!url_.is_empty()); + DCHECK(url_request_context_getter_); + auto request = base::MakeUnique<JsonRequest>( + params_.exclusive_category, tick_clock_, parse_json_callback_); + std::string body = BuildBody(); + std::string headers = BuildHeaders(); + request->url_fetcher_ = BuildURLFetcher(request.get(), headers, body); + + // Log the request for debugging network issues. + VLOG(1) << "Sending a NTP snippets request to " << url_ << ":\n" + << headers << "\n" + << body; + + return request; +} + +NTPSnippetsFetcher::RequestBuilder& +NTPSnippetsFetcher::RequestBuilder::SetAuthentication( + const std::string& account_id, + const std::string& auth_header) { + obfuscated_gaia_id_ = account_id; + auth_header_ = auth_header; + return *this; +} + +NTPSnippetsFetcher::RequestBuilder& +NTPSnippetsFetcher::RequestBuilder::SetFetchAPI(FetchAPI fetch_api) { + fetch_api_ = fetch_api; + return *this; +} + +NTPSnippetsFetcher::RequestBuilder& +NTPSnippetsFetcher::RequestBuilder::SetLanguageModel( + const translate::LanguageModel* language_model) { + language_model_ = language_model; + return *this; +} + +NTPSnippetsFetcher::RequestBuilder& +NTPSnippetsFetcher::RequestBuilder::SetParams(const Params& params) { + params_ = params; + return *this; +} + +NTPSnippetsFetcher::RequestBuilder& +NTPSnippetsFetcher::RequestBuilder::SetParseJsonCallback( + ParseJSONCallback callback) { + parse_json_callback_ = callback; + return *this; +} + +NTPSnippetsFetcher::RequestBuilder& +NTPSnippetsFetcher::RequestBuilder::SetPersonalization( + Personalization personalization) { + personalization_ = personalization; + return *this; +} + +NTPSnippetsFetcher::RequestBuilder& +NTPSnippetsFetcher::RequestBuilder::SetTickClock(base::TickClock* tick_clock) { + tick_clock_ = tick_clock; + return *this; +} + +NTPSnippetsFetcher::RequestBuilder& NTPSnippetsFetcher::RequestBuilder::SetUrl( + const GURL& url) { + url_ = url; + return *this; +} + +NTPSnippetsFetcher::RequestBuilder& +NTPSnippetsFetcher::RequestBuilder::SetUrlRequestContextGetter( + const scoped_refptr<net::URLRequestContextGetter>& context_getter) { + url_request_context_getter_ = context_getter; + return *this; +} + +NTPSnippetsFetcher::RequestBuilder& +NTPSnippetsFetcher::RequestBuilder::SetUserClassifier( + const UserClassifier& user_classifier) { + if (IsSendingUserClassEnabled()) { + user_class_ = GetUserClassString(user_classifier.GetUserClass()); + } + return *this; +} + +bool NTPSnippetsFetcher::RequestBuilder::IsSendingTopLanguagesEnabled() const { + return IsBooleanParameterEnabled(kSendTopLanguagesName, + /*default_value=*/false); +} + +bool NTPSnippetsFetcher::RequestBuilder::IsSendingUserClassEnabled() const { + return IsBooleanParameterEnabled(kSendUserClassName, + /*default_value=*/false); +} + +std::string NTPSnippetsFetcher::RequestBuilder::BuildHeaders() const { + net::HttpRequestHeaders headers; + headers.SetHeader("Content-Type", "application/json; charset=UTF-8"); + if (!auth_header_.empty()) { + headers.SetHeader("Authorization", auth_header_); + } + // Add X-Client-Data header with experiment IDs from field trials. + variations::AppendVariationHeaders(url_, + false, // incognito + false, // uma_enabled + &headers); + return headers.ToString(); +} + +std::string NTPSnippetsFetcher::RequestBuilder::BuildBody() const { auto request = base::MakeUnique<base::DictionaryValue>(); - std::string user_locale = PosixLocaleFromBCP47Language(params.language_code); - switch (fetch_api) { - case CHROME_READER_API: { + std::string user_locale = PosixLocaleFromBCP47Language(params_.language_code); + switch (fetch_api_) { + case NTPSnippetsFetcher::CHROME_READER_API: { auto content_params = base::MakeUnique<base::DictionaryValue>(); content_params->SetBoolean("only_return_personalized_results", - only_return_personalized_results); + ReturnOnlyPersonalizedResults()); auto content_restricts = base::MakeUnique<base::ListValue>(); for (const auto* metadata : {"TITLE", "SNIPPET", "THUMBNAIL"}) { @@ -437,7 +703,7 @@ } auto content_selectors = base::MakeUnique<base::ListValue>(); - for (const auto& host : params.hosts) { + for (const auto& host : params_.hosts) { auto entry = base::MakeUnique<base::DictionaryValue>(); entry->SetString("type", "HOST_RESTRICT"); entry->SetString("value", host); @@ -452,7 +718,8 @@ std::move(content_selectors)); auto global_scoring_params = base::MakeUnique<base::DictionaryValue>(); - global_scoring_params->SetInteger("num_to_return", params.count_to_fetch); + global_scoring_params->SetInteger("num_to_return", + params_.count_to_fetch); global_scoring_params->SetInteger("sort_type", 1); auto advanced = base::MakeUnique<base::DictionaryValue>(); @@ -461,8 +728,8 @@ request->SetString("response_detail_level", "STANDARD"); request->Set("advanced_options", std::move(advanced)); - if (!obfuscated_gaia_id.empty()) { - request->SetString("obfuscated_gaia_id", obfuscated_gaia_id); + if (!obfuscated_gaia_id_.empty()) { + request->SetString("obfuscated_gaia_id", obfuscated_gaia_id_); } if (!user_locale.empty()) { request->SetString("user_locale", user_locale); @@ -470,22 +737,22 @@ break; } - case CHROME_CONTENT_SUGGESTIONS_API: { + case NTPSnippetsFetcher::CHROME_CONTENT_SUGGESTIONS_API: { if (!user_locale.empty()) { request->SetString("uiLanguage", user_locale); } auto regular_hosts = base::MakeUnique<base::ListValue>(); - for (const auto& host : params.hosts) { + for (const auto& host : params_.hosts) { regular_hosts->AppendString(host); } request->Set("regularlyVisitedHostNames", std::move(regular_hosts)); - request->SetString("priority", params.interactive_request + request->SetString("priority", params_.interactive_request ? "USER_ACTION" : "BACKGROUND_PREFETCH"); auto excluded = base::MakeUnique<base::ListValue>(); - for (const auto& id : params.excluded_ids) { + for (const auto& id : params_.excluded_ids) { excluded->AppendString(id); if (excluded->GetSize() >= kMaxExcludedIds) { break; @@ -493,10 +760,14 @@ } request->Set("excludedSuggestionIds", std::move(excluded)); - if (!user_class.empty()) { - request->SetString("userActivenessClass", user_class); + if (!user_class_.empty()) { + request->SetString("userActivenessClass", user_class_); } + translate::LanguageModel::LanguageInfo ui_language; + translate::LanguageModel::LanguageInfo other_top_language; + PrepareLanguages(&ui_language, &other_top_language); + if (ui_language.frequency == 0 && other_top_language.frequency == 0) { break; } @@ -523,115 +794,100 @@ return request_json; } -void NTPSnippetsFetcher::FetchSnippetsImpl(const GURL& url, - const std::string& auth_header, - const std::string& request) { - url_fetcher_ = URLFetcher::Create(url, URLFetcher::POST, this); - - url_fetcher_->SetRequestContext(url_request_context_getter_.get()); - url_fetcher_->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES | - net::LOAD_DO_NOT_SAVE_COOKIES); - +std::unique_ptr<net::URLFetcher> +NTPSnippetsFetcher::RequestBuilder::BuildURLFetcher( + net::URLFetcherDelegate* delegate, + const std::string& headers, + const std::string& body) const { + std::unique_ptr<net::URLFetcher> url_fetcher = + net::URLFetcher::Create(url_, net::URLFetcher::POST, delegate); + url_fetcher->SetRequestContext(url_request_context_getter_.get()); + url_fetcher->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES | + net::LOAD_DO_NOT_SAVE_COOKIES); data_use_measurement::DataUseUserData::AttachToFetcher( - url_fetcher_.get(), data_use_measurement::DataUseUserData::NTP_SNIPPETS); + url_fetcher.get(), data_use_measurement::DataUseUserData::NTP_SNIPPETS); - HttpRequestHeaders headers; - if (!auth_header.empty()) { - headers.SetHeader("Authorization", auth_header); - } - headers.SetHeader("Content-Type", "application/json; charset=UTF-8"); - // Add X-Client-Data header with experiment IDs from field trials. - variations::AppendVariationHeaders(url, - false, // incognito - false, // uma_enabled - &headers); - url_fetcher_->SetExtraRequestHeaders(headers.ToString()); - url_fetcher_->SetUploadData("application/json", request); - // Log the request for debugging network issues. - VLOG(1) << "Sending a NTP snippets request to " << url << ":" << std::endl - << headers.ToString() << std::endl - << request; + url_fetcher->SetExtraRequestHeaders(headers); + url_fetcher->SetUploadData("application/json", body); + // Fetchers are sometimes cancelled because a network change was detected. - url_fetcher_->SetAutomaticallyRetryOnNetworkChanges(3); + url_fetcher->SetAutomaticallyRetryOnNetworkChanges(3); // Try to make fetching the files bit more robust even with poor connection. - url_fetcher_->SetMaxRetriesOn5xx(3); - url_fetcher_->Start(); + url_fetcher->SetMaxRetriesOn5xx(3); + return url_fetcher; } -bool NTPSnippetsFetcher::UsesAuthentication() const { - return (personalization_ == Personalization::kPersonal || - personalization_ == Personalization::kBoth); -} - -NTPSnippetsFetcher::RequestBuilder NTPSnippetsFetcher::MakeRequestBuilder() - const { - RequestBuilder result; - result.params = params_; - result.fetch_api = fetch_api_; - - if (IsSendingUserClassEnabled()) { - result.user_class = GetUserClassString(user_classifier_->GetUserClass()); - } - +void NTPSnippetsFetcher::RequestBuilder::PrepareLanguages( + translate::LanguageModel::LanguageInfo* ui_language, + translate::LanguageModel::LanguageInfo* other_top_language) const { // TODO(jkrcal): Add language model factory for iOS and add fakes to tests so - // that |language_model_| is never nullptr. Remove this check and add a DCHECK + // that |language_model| is never nullptr. Remove this check and add a DCHECK // into the constructor. if (!language_model_ || !IsSendingTopLanguagesEnabled()) { - return result; + return; } // TODO(jkrcal): Is this back-and-forth converting necessary? - result.ui_language.language_code = ISO639FromPosixLocale( - PosixLocaleFromBCP47Language(result.params.language_code)); - result.ui_language.frequency = - language_model_->GetLanguageFrequency(result.ui_language.language_code); + ui_language->language_code = ISO639FromPosixLocale( + PosixLocaleFromBCP47Language(params_.language_code)); + ui_language->frequency = + language_model_->GetLanguageFrequency(ui_language->language_code); std::vector<LanguageModel::LanguageInfo> top_languages = language_model_->GetTopLanguages(); for (const LanguageModel::LanguageInfo& info : top_languages) { - if (info.language_code != result.ui_language.language_code) { - result.other_top_language = info; + if (info.language_code != ui_language->language_code) { + *other_top_language = info; // Report to UMA how important the UI language is. - DCHECK_GT(result.other_top_language.frequency, 0) + DCHECK_GT(other_top_language->frequency, 0) << "GetTopLanguages() should not return languages with 0 frequency"; float ratio_ui_in_both_languages = - result.ui_language.frequency / - (result.ui_language.frequency + result.other_top_language.frequency); + ui_language->frequency / + (ui_language->frequency + other_top_language->frequency); UMA_HISTOGRAM_PERCENTAGE( "NewTabPage.Languages.UILanguageRatioInTwoTopLanguages", ratio_ui_in_both_languages * 100); break; } } - - return result; } -void NTPSnippetsFetcher::FetchSnippetsNonAuthenticated() { +void NTPSnippetsFetcher::FetchSnippetsNonAuthenticated( + RequestBuilder builder, + SnippetsAvailableCallback callback) { // When not providing OAuth token, we need to pass the Google API key. - GURL url(base::StringPrintf(kSnippetsServerNonAuthorizedFormat, - fetch_url_.spec().c_str(), api_key_.c_str())); - FetchSnippetsImpl(url, std::string(), MakeRequestBuilder().BuildRequest()); + builder.SetUrl( + GURL(base::StringPrintf(kSnippetsServerNonAuthorizedFormat, + fetch_url_.spec().c_str(), api_key_.c_str()))); + StartRequest(std::move(builder), std::move(callback)); } void NTPSnippetsFetcher::FetchSnippetsAuthenticated( + RequestBuilder builder, + SnippetsAvailableCallback callback, const std::string& account_id, const std::string& oauth_access_token) { - RequestBuilder builder = MakeRequestBuilder(); - builder.obfuscated_gaia_id = account_id; - builder.only_return_personalized_results = - personalization_ == Personalization::kPersonal; // TODO(jkrcal, treib): Add unit-tests for authenticated fetches. - FetchSnippetsImpl(fetch_url_, - base::StringPrintf(kAuthorizationRequestHeaderFormat, - oauth_access_token.c_str()), - builder.BuildRequest()); + builder.SetUrl(fetch_url_) + .SetAuthentication(account_id, + base::StringPrintf(kAuthorizationRequestHeaderFormat, + oauth_access_token.c_str())); + StartRequest(std::move(builder), std::move(callback)); +} + +void NTPSnippetsFetcher::StartRequest(RequestBuilder builder, + SnippetsAvailableCallback callback) { + std::unique_ptr<JsonRequest> request = builder.Build(); + JsonRequest* raw_request = request.get(); + raw_request->Start(base::BindOnce(&NTPSnippetsFetcher::JsonRequestDone, + base::Unretained(this), std::move(request), + std::move(callback))); } void NTPSnippetsFetcher::StartTokenRequest() { OAuth2TokenService::ScopeSet scopes; - scopes.insert(fetch_api_ == CHROME_CONTENT_SUGGESTIONS_API + scopes.insert(fetch_api_ == FetchAPI::CHROME_CONTENT_SUGGESTIONS_API ? kContentSuggestionsApiScope : kChromeReaderApiScope); oauth_request_ = token_service_->StartRequest( @@ -650,7 +906,14 @@ DCHECK_EQ(oauth_request.get(), request) << "Got tokens from some previous request"; - FetchSnippetsAuthenticated(oauth_request->GetAccountId(), access_token); + while (!pending_requests_.empty()) { + std::pair<RequestBuilder, SnippetsAvailableCallback> builder_and_callback = + std::move(pending_requests_.front()); + pending_requests_.pop(); + FetchSnippetsAuthenticated(std::move(builder_and_callback.first), + std::move(builder_and_callback.second), + oauth_request->GetAccountId(), access_token); + } } void NTPSnippetsFetcher::OnGetTokenFailure( @@ -668,9 +931,17 @@ } DLOG(ERROR) << "Unable to get token: " << error.ToString(); - FetchFinished( - OptionalFetchedCategories(), FetchResult::OAUTH_TOKEN_ERROR, - /*extra_message=*/base::StringPrintf(" (%s)", error.ToString().c_str())); + while (!pending_requests_.empty()) { + std::pair<RequestBuilder, SnippetsAvailableCallback> builder_and_callback = + std::move(pending_requests_.front()); + + FetchFinished(OptionalFetchedCategories(), + std::move(builder_and_callback.second), + FetchResult::OAUTH_TOKEN_ERROR, + /*error_details=*/base::StringPrintf( + " (%s)", error.ToString().c_str())); + pending_requests_.pop(); + } } //////////////////////////////////////////////////////////////////////////////// @@ -688,42 +959,53 @@ StartTokenRequest(); } -//////////////////////////////////////////////////////////////////////////////// -// URLFetcherDelegate overrides -void NTPSnippetsFetcher::OnURLFetchComplete(const URLFetcher* source) { - DCHECK_EQ(url_fetcher_.get(), source); - std::unique_ptr<URLFetcher> deleter = std::move(url_fetcher_); +void NTPSnippetsFetcher::JsonRequestDone(std::unique_ptr<JsonRequest> request, + SnippetsAvailableCallback callback, + std::unique_ptr<base::Value> result, + FetchResult status_code, + const std::string& error_details) { + DCHECK(request); + last_fetch_json_ = request->GetResponseString(); - const URLRequestStatus& status = source->GetStatus(); + UMA_HISTOGRAM_TIMES("NewTabPage.Snippets.FetchTime", + request->GetFetchDuration()); - UMA_HISTOGRAM_SPARSE_SLOWLY( - "NewTabPage.Snippets.FetchHttpResponseOrErrorCode", - status.is_success() ? source->GetResponseCode() : status.error()); - - if (!status.is_success()) { - FetchFinished(OptionalFetchedCategories(), - FetchResult::URL_REQUEST_STATUS_ERROR, - /*extra_message=*/base::StringPrintf(" %d", status.error())); - } else if (source->GetResponseCode() != net::HTTP_OK) { - // TODO(jkrcal): https://crbug.com/609084 - // We need to deal with the edge case again where the auth - // token expires just before we send the request (in which case we need to - // fetch a new auth token). We should extract that into a common class - // instead of adding it to every single class that uses auth tokens. - FetchFinished( - OptionalFetchedCategories(), FetchResult::HTTP_ERROR, - /*extra_message=*/base::StringPrintf(" %d", source->GetResponseCode())); - } else { - bool stores_result_to_string = - source->GetResponseAsString(&last_fetch_json_); - DCHECK(stores_result_to_string); - - parse_json_callback_.Run(last_fetch_json_, - base::Bind(&NTPSnippetsFetcher::OnJsonParsed, - weak_ptr_factory_.GetWeakPtr()), - base::Bind(&NTPSnippetsFetcher::OnJsonError, - weak_ptr_factory_.GetWeakPtr())); + if (!result) { + FetchFinished(OptionalFetchedCategories(), std::move(callback), status_code, + error_details); + return; } + FetchedCategoriesVector categories; + if (!JsonToSnippets(*result, &categories)) { + LOG(WARNING) << "Received invalid snippets: " << last_fetch_json_; + FetchFinished(OptionalFetchedCategories(), std::move(callback), + FetchResult::INVALID_SNIPPET_CONTENT_ERROR, std::string()); + return; + } + // Filter out unwanted categories if necessary. + // TODO(fhorschig): As soon as the server supports filtering by category, + // adjust the request instead of over-fetching and filtering here. + FilterCategories(&categories, request->exclusive_category()); + + FetchFinished(std::move(categories), std::move(callback), + FetchResult::SUCCESS, std::string()); +} + +void NTPSnippetsFetcher::FetchFinished(OptionalFetchedCategories categories, + SnippetsAvailableCallback callback, + FetchResult status_code, + const std::string& error_details) { + DCHECK(status_code == FetchResult::SUCCESS || !categories.has_value()); + + last_status_ = FetchResultToString(status_code) + error_details; + + UMA_HISTOGRAM_ENUMERATION("NewTabPage.Snippets.FetchResult", + static_cast<int>(status_code), + static_cast<int>(FetchResult::RESULT_MAX)); + + DVLOG(1) << "Fetch finished: " << last_status_; + + std::move(callback).Run(std::move(categories)); } bool NTPSnippetsFetcher::JsonToSnippets(const base::Value& parsed, @@ -734,7 +1016,7 @@ } switch (fetch_api_) { - case CHROME_READER_API: { + case FetchAPI::CHROME_READER_API: { const int kUnusedRemoteCategoryId = -1; categories->push_back(FetchedCategory( category_factory_->FromKnownCategory(KnownCategories::ARTICLES), @@ -743,11 +1025,11 @@ const base::ListValue* recos = nullptr; return top_dict->GetList("recos", &recos) && AddSnippetsFromListValue(/*content_suggestions_api=*/false, - kUnusedRemoteCategoryId, - *recos, &categories->back().snippets); + kUnusedRemoteCategoryId, *recos, + &categories->back().snippets); } - case CHROME_CONTENT_SUGGESTIONS_API: { + case FetchAPI::CHROME_CONTENT_SUGGESTIONS_API: { const base::ListValue* categories_value = nullptr; if (!top_dict->GetList("categories", &categories_value)) { return false; @@ -770,9 +1052,9 @@ // is permissible. if (category_value->GetList("suggestions", &suggestions)) { if (!AddSnippetsFromListValue( - /*content_suggestions_api=*/true, remote_category_id, - *suggestions, &snippets)) { - return false; + /*content_suggestions_api=*/true, remote_category_id, + *suggestions, &snippets)) { + return false; } } Category category = @@ -800,80 +1082,6 @@ return false; } -void NTPSnippetsFetcher::OnJsonParsed(std::unique_ptr<base::Value> parsed) { - FetchedCategoriesVector categories; - if (JsonToSnippets(*parsed, &categories)) { - FetchFinished(OptionalFetchedCategories(std::move(categories)), - FetchResult::SUCCESS, - /*extra_message=*/std::string()); - } else { - LOG(WARNING) << "Received invalid snippets: " << last_fetch_json_; - FetchFinished(OptionalFetchedCategories(), - FetchResult::INVALID_SNIPPET_CONTENT_ERROR, - /*extra_message=*/std::string()); - } -} - -void NTPSnippetsFetcher::OnJsonError(const std::string& error) { - LOG(WARNING) << "Received invalid JSON (" << error - << "): " << last_fetch_json_; - FetchFinished( - OptionalFetchedCategories(), FetchResult::JSON_PARSE_ERROR, - /*extra_message=*/base::StringPrintf(" (error %s)", error.c_str())); -} - -// The response from the backend might include suggestions from multiple -// categories. If only fetches for a single category were requested, this -// function filters them out. -void NTPSnippetsFetcher::FilterCategories(FetchedCategoriesVector* categories) { - if (!params_.exclusive_category.has_value()) { - return; - } - Category exclusive = params_.exclusive_category.value(); - auto category_it = - std::find_if(categories->begin(), categories->end(), - [&exclusive](const FetchedCategory& c) -> bool { - return c.category == exclusive; - }); - if (category_it == categories->end()) { - categories->clear(); - return; - } - FetchedCategory category = std::move(*category_it); - categories->clear(); - categories->emplace_back(std::move(category)); -} - -void NTPSnippetsFetcher::FetchFinished( - OptionalFetchedCategories fetched_categories, - FetchResult result, - const std::string& extra_message) { - DCHECK(result == FetchResult::SUCCESS || !fetched_categories); - last_status_ = FetchResultToString(result) + extra_message; - - // Filter out unwanted categories if necessary. - // TODO(fhorschig): As soon as the server supports filtering by - // that instead of over-fetching and filtering here. - if (fetched_categories.has_value()) { - FilterCategories(&fetched_categories.value()); - } - - // Don't record FetchTimes if the result indicates that a precondition - // failed and we never actually sent a network request - if (!IsFetchPreconditionFailed(result)) { - UMA_HISTOGRAM_TIMES("NewTabPage.Snippets.FetchTime", - tick_clock_->NowTicks() - fetch_start_time_); - } - UMA_HISTOGRAM_ENUMERATION("NewTabPage.Snippets.FetchResult", - static_cast<int>(result), - static_cast<int>(FetchResult::RESULT_MAX)); - - DVLOG(1) << "Fetch finished: " << last_status_; - if (!snippets_available_callback_.is_null()) { - std::move(snippets_available_callback_).Run(std::move(fetched_categories)); - } -} - bool NTPSnippetsFetcher::DemandQuotaForRequest(bool interactive_request) { switch (user_classifier_->GetUserClass()) { case UserClassifier::UserClass::RARE_NTP_USER: @@ -890,4 +1098,9 @@ return false; } +bool NTPSnippetsFetcher::NeedsAuthentication() const { + return (personalization_ == Personalization::kPersonal || + personalization_ == Personalization::kBoth); +} + } // namespace ntp_snippets
diff --git a/components/ntp_snippets/remote/ntp_snippets_fetcher.h b/components/ntp_snippets/remote/ntp_snippets_fetcher.h index 7816314..62437c9 100644 --- a/components/ntp_snippets/remote/ntp_snippets_fetcher.h +++ b/components/ntp_snippets/remote/ntp_snippets_fetcher.h
@@ -6,6 +6,7 @@ #define COMPONENTS_NTP_SNIPPETS_REMOTE_NTP_SNIPPETS_FETCHER_H_ #include <memory> +#include <queue> #include <set> #include <string> #include <utility> @@ -22,7 +23,7 @@ #include "components/ntp_snippets/remote/request_throttler.h" #include "components/translate/core/browser/language_model.h" #include "google_apis/gaia/oauth2_token_service.h" -#include "net/url_request/url_fetcher_delegate.h" +#include "net/http/http_request_headers.h" #include "net/url_request/url_request_context_getter.h" class PrefService; @@ -54,15 +55,17 @@ // Fetches snippet data for the NTP from the server. class NTPSnippetsFetcher : public OAuth2TokenService::Consumer, - public OAuth2TokenService::Observer, - public net::URLFetcherDelegate { + public OAuth2TokenService::Observer { public: // Callbacks for JSON parsing, needed because the parsing is platform- // dependent. - using SuccessCallback = base::Callback<void(std::unique_ptr<base::Value>)>; - using ErrorCallback = base::Callback<void(const std::string&)>; - using ParseJSONCallback = base::Callback< - void(const std::string&, const SuccessCallback&, const ErrorCallback&)>; + using SuccessCallback = + base::Callback<void(std::unique_ptr<base::Value> result)>; + using ErrorCallback = base::Callback<void(const std::string& error)>; + using ParseJSONCallback = + base::Callback<void(const std::string& raw_json_string, + const SuccessCallback& success_callback, + const ErrorCallback& error_callback)>; struct FetchedCategory { Category category; @@ -100,11 +103,7 @@ }; // Enumeration listing all possible variants of dealing with personalization. - enum class Personalization { - kPersonal, - kNonPersonal, - kBoth - }; + enum class Personalization { kPersonal, kNonPersonal, kBoth }; // Contains all the parameters for one fetch. struct Params { @@ -167,9 +166,6 @@ // Returns the URL endpoint used by the fetcher. const GURL& fetch_url() const { return fetch_url_; } - // Does the fetcher use authentication to get personalized results? - bool UsesAuthentication() const; - // Overrides internal clock for testing purposes. void SetTickClockForTesting(std::unique_ptr<base::TickClock> tick_clock) { tick_clock_ = std::move(tick_clock); @@ -186,36 +182,103 @@ BuildRequestWithUILanguageOnly); FRIEND_TEST_ALL_PREFIXES(NTPSnippetsFetcherTest, BuildRequestWithOtherLanguageOnly); + friend class NTPSnippetsFetcherTest; enum FetchAPI { CHROME_READER_API, CHROME_CONTENT_SUGGESTIONS_API, }; - struct RequestBuilder { - Params params; - FetchAPI fetch_api = CHROME_READER_API; - std::string obfuscated_gaia_id; - bool only_return_personalized_results = false; - std::string user_class; - translate::LanguageModel::LanguageInfo ui_language; - translate::LanguageModel::LanguageInfo other_top_language; + class JsonRequest; + // A class that builds authenticated and non-authenticated JsonRequests. + // This class is only in the header for testing. + // TODO(fhorschig): Move into separate file with snippets::internal namespace. + class RequestBuilder { + public: RequestBuilder(); RequestBuilder(RequestBuilder&&); ~RequestBuilder(); - std::string BuildRequest(); + // Builds a Request object that contains all data to fetch new snippets. + std::unique_ptr<JsonRequest> Build() const; + + RequestBuilder& SetAuthentication(const std::string& account_id, + const std::string& auth_header); + RequestBuilder& SetCreationTime(base::TimeTicks creation_time); + RequestBuilder& SetFetchAPI(FetchAPI fetch_api); + // The language_model borrowed from the fetcher needs to stay alive until + // the request body is built. + RequestBuilder& SetLanguageModel( + const translate::LanguageModel* language_model); + RequestBuilder& SetParams(const Params& params); + RequestBuilder& SetParseJsonCallback(ParseJSONCallback callback); + RequestBuilder& SetPersonalization(Personalization personalization); + // The tick_clock borrowed from the fetcher will be injected into the + // request. It will be used at build time and after the fetch returned. + // It has to be alive until the request is destroyed. + RequestBuilder& SetTickClock(base::TickClock* tick_clock); + RequestBuilder& SetUrl(const GURL& url); + RequestBuilder& SetUrlRequestContextGetter( + const scoped_refptr<net::URLRequestContextGetter>& context_getter); + RequestBuilder& SetUserClassifier(const UserClassifier& user_classifier); + + std::string PreviewRequestBodyForTesting() { return BuildBody(); } + std::string PreviewRequestHeadersForTesting() { return BuildHeaders(); } + RequestBuilder& SetUserClassForTesting(const std::string& user_class) { + user_class_ = user_class; + return *this; + } + + protected: + // TODO(fhorschig): As soon as crbug.com/crbug.com/645447 is resolved, + // make these an implementation detail in the body and remove the mock. + virtual bool IsSendingTopLanguagesEnabled() const; + virtual bool IsSendingUserClassEnabled() const; + + private: + std::string BuildHeaders() const; + std::string BuildBody() const; + std::unique_ptr<net::URLFetcher> BuildURLFetcher( + net::URLFetcherDelegate* request, + const std::string& headers, + const std::string& body) const; + + bool ReturnOnlyPersonalizedResults() const { + return !obfuscated_gaia_id_.empty() && + personalization_ == NTPSnippetsFetcher::Personalization::kPersonal; + } + + void PrepareLanguages( + translate::LanguageModel::LanguageInfo* ui_language, + translate::LanguageModel::LanguageInfo* other_top_language) const; + + // Only required, if the request needs to be sent. + std::string auth_header_; + base::TickClock* tick_clock_; + FetchAPI fetch_api_; + Params params_; + ParseJSONCallback parse_json_callback_; + Personalization personalization_; + GURL url_; + scoped_refptr<net::URLRequestContextGetter> url_request_context_getter_; + + // Optional properties. + std::string obfuscated_gaia_id_; + std::string user_class_; + const translate::LanguageModel* language_model_; + + DISALLOW_COPY_AND_ASSIGN(RequestBuilder); }; - RequestBuilder MakeRequestBuilder() const; - - void FetchSnippetsImpl(const GURL& url, - const std::string& auth_header, - const std::string& request); - void FetchSnippetsNonAuthenticated(); - void FetchSnippetsAuthenticated(const std::string& account_id, + void FetchSnippetsNonAuthenticated(RequestBuilder builder, + SnippetsAvailableCallback callback); + void FetchSnippetsAuthenticated(RequestBuilder builder, + SnippetsAvailableCallback callback, + const std::string& account_id, const std::string& oauth_access_token); + void StartRequest(RequestBuilder builder, SnippetsAvailableCallback callback); + void StartTokenRequest(); // OAuth2TokenService::Consumer overrides: @@ -228,20 +291,24 @@ // OAuth2TokenService::Observer overrides: void OnRefreshTokenAvailable(const std::string& account_id) override; - // URLFetcherDelegate implementation. - void OnURLFetchComplete(const net::URLFetcher* source) override; + void JsonRequestDone(std::unique_ptr<JsonRequest> request, + SnippetsAvailableCallback callback, + std::unique_ptr<base::Value> result, + FetchResult status_code, + const std::string& error_details); + void FetchFinished(OptionalFetchedCategories categories, + SnippetsAvailableCallback callback, + FetchResult status_code, + const std::string& error_details); bool JsonToSnippets(const base::Value& parsed, - FetchedCategoriesVector* categories); - void OnJsonParsed(std::unique_ptr<base::Value> parsed); - void OnJsonError(const std::string& error); - void FetchFinished(OptionalFetchedCategories fetched_categories, - FetchResult result, - const std::string& extra_message); - void FilterCategories(FetchedCategoriesVector* categories); + NTPSnippetsFetcher::FetchedCategoriesVector* categories); bool DemandQuotaForRequest(bool interactive_request); + // Does the fetcher use authentication to get personalized results? + bool NeedsAuthentication() const; + // Authentication for signed-in users. SigninManagerBase* signin_manager_; OAuth2TokenService* token_service_; @@ -254,6 +321,10 @@ // Holds the URL request context. scoped_refptr<net::URLRequestContextGetter> url_request_context_getter_; + // Stores requests that wait for an access token. + std::queue<std::pair<RequestBuilder, SnippetsAvailableCallback>> + pending_requests_; + // Weak references, not owned. CategoryFactory* const category_factory_; translate::LanguageModel* const language_model_; @@ -282,19 +353,6 @@ RequestThrottler request_throttler_active_ntp_user_; RequestThrottler request_throttler_active_suggestions_consumer_; - // The callback to notify when new snippets get fetched. - SnippetsAvailableCallback snippets_available_callback_; - - // The parameters for the current request. - Params params_; - - // The fetcher for downloading the snippets. Only non-null if a fetch is - // currently ongoing. - std::unique_ptr<net::URLFetcher> url_fetcher_; - - // When the current request was started, for logging purposes. - base::TimeTicks fetch_start_time_; - // Info on the last finished fetch. std::string last_status_; std::string last_fetch_json_;
diff --git a/components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc b/components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc index 6c841065..e5533de8bd 100644 --- a/components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc +++ b/components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc
@@ -45,7 +45,9 @@ using testing::NotNull; using testing::Pointee; using testing::PrintToString; +using testing::Return; using testing::StartsWith; +using testing::StrEq; using testing::WithArg; const char kAPIKey[] = "fakeAPIkey"; @@ -186,7 +188,8 @@ const ntp_snippets::NTPSnippetsFetcher::SuccessCallback& success_callback, const ntp_snippets::NTPSnippetsFetcher::ErrorCallback& error_callback) { base::ThreadTaskRunnerHandle::Get()->PostDelayedTask( - FROM_HERE, base::Bind(&ParseJson, json, success_callback, error_callback), + FROM_HERE, base::Bind(&ParseJson, json, std::move(success_callback), + std::move(error_callback)), base::TimeDelta::FromMilliseconds(kTestJsonParsingLatencyMs)); } @@ -194,26 +197,48 @@ class NTPSnippetsFetcherTest : public testing::Test { public: + // TODO(fhorschig): As soon as crbug.com/645447 is resolved, use + // variations::testing::VariationParamsManager to configure these values. + class RequestBuilderWithMockedFlagsForTesting + : public NTPSnippetsFetcher::RequestBuilder { + public: + RequestBuilderWithMockedFlagsForTesting() : RequestBuilder() {} + + private: + bool IsSendingUserClassEnabled() const override { return true; } + bool IsSendingTopLanguagesEnabled() const override { return true; } + }; + NTPSnippetsFetcherTest() : NTPSnippetsFetcherTest(GURL(kTestChromeReaderUrl), std::map<std::string, std::string>()) {} NTPSnippetsFetcherTest(const GURL& gurl, const std::map<std::string, std::string>& params) - : params_manager_(ntp_snippets::kStudyName, params), + : params_manager_( + base::MakeUnique<variations::testing::VariationParamsManager>( + ntp_snippets::kStudyName, + params)), mock_task_runner_(new base::TestMockTimeTaskRunner()), mock_task_runner_handle_(mock_task_runner_), - signin_client_(new TestSigninClient(nullptr)), - account_tracker_(new AccountTrackerService()), - fake_signin_manager_(new FakeSigninManagerBase(signin_client_.get(), - account_tracker_.get())), - fake_token_service_(new FakeProfileOAuth2TokenService()), - pref_service_(new TestingPrefServiceSimple()), + signin_client_(base::MakeUnique<TestSigninClient>(nullptr)), + account_tracker_(base::MakeUnique<AccountTrackerService>()), + fake_signin_manager_( + base::MakeUnique<FakeSigninManagerBase>(signin_client_.get(), + account_tracker_.get())), + fake_token_service_(base::MakeUnique<FakeProfileOAuth2TokenService>()), + pref_service_(base::MakeUnique<TestingPrefServiceSimple>()), test_url_(gurl) { RequestThrottler::RegisterProfilePrefs(pref_service_->registry()); UserClassifier::RegisterProfilePrefs(pref_service_->registry()); + translate::LanguageModel::RegisterProfilePrefs(pref_service()->registry()); user_classifier_ = base::MakeUnique<UserClassifier>(pref_service_.get()); + // Increase initial time such that ticks are non-zero. + mock_task_runner_->FastForwardBy(base::TimeDelta::FromMilliseconds(1234)); + ResetSnippetsFetcher(); + } + void ResetSnippetsFetcher() { snippets_fetcher_ = base::MakeUnique<NTPSnippetsFetcher>( fake_signin_manager_.get(), fake_token_service_.get(), scoped_refptr<net::TestURLRequestContextGetter>( @@ -223,8 +248,6 @@ snippets_fetcher_->SetTickClockForTesting( mock_task_runner_->GetMockTickClock()); - // Increase initial time such that ticks are non-zero. - mock_task_runner_->FastForwardBy(base::TimeDelta::FromMilliseconds(1234)); } NTPSnippetsFetcher::SnippetsAvailableCallback ToSnippetsAvailableCallback( @@ -257,6 +280,16 @@ /*default_factory=*/&failing_url_fetcher_factory_)); } + void SetFetchingPersonalizationVariation( + const std::string& personalization_string) { + params_manager_.reset(); + std::map<std::string, std::string> params = { + {"fetching_personalization", personalization_string}}; + params_manager_ = + base::MakeUnique<variations::testing::VariationParamsManager>( + ntp_snippets::kStudyName, params); + } + void SetFakeResponse(const std::string& response_data, net::HttpStatusCode response_code, net::URLRequestStatus::Status status) { @@ -265,8 +298,24 @@ response_code, status); } + std::unique_ptr<translate::LanguageModel> MakeLanguageModel( + const std::vector<std::string>& codes) { + std::unique_ptr<translate::LanguageModel> language_model = + base::MakeUnique<translate::LanguageModel>(pref_service()); + // There must be at least 10 visits before the top languages are defined. + for (int i = 0; i < 10; i++) { + for (const auto& code : codes) { + language_model->OnPageVisited(code); + } + } + return language_model; + } + + TestingPrefServiceSimple* pref_service() const { return pref_service_.get(); } + private: - variations::testing::VariationParamsManager params_manager_; + // TODO(fhorschig): Make it a simple member as soon as it resets properly. + std::unique_ptr<variations::testing::VariationParamsManager> params_manager_; scoped_refptr<base::TestMockTimeTaskRunner> mock_task_runner_; base::ThreadTaskRunnerHandle mock_task_runner_handle_; FailingFakeURLFetcherFactory failing_url_fetcher_factory_; @@ -297,21 +346,25 @@ TEST_F(NTPSnippetsFetcherTest, BuildRequestAuthenticated) { NTPSnippetsFetcher::RequestBuilder builder; - builder.params.hosts = {"chromium.org"}; - builder.params.excluded_ids = {"1234567890"}; - builder.params.count_to_fetch = 25; - builder.params.language_code = "en"; - builder.params.interactive_request = false; - builder.obfuscated_gaia_id = "0BFUSGAIA"; - builder.only_return_personalized_results = true; - builder.user_class = "ACTIVE_NTP_USER"; + NTPSnippetsFetcher::Params params; + params.hosts = {"chromium.org"}; + params.excluded_ids = {"1234567890"}; + params.count_to_fetch = 25; + params.interactive_request = false; + builder.SetParams(params) + .SetAuthentication("0BFUSGAIA", "headerstuff") + .SetPersonalization(NTPSnippetsFetcher::Personalization::kPersonal) + .SetUserClassForTesting("ACTIVE_NTP_USER") + .SetFetchAPI(NTPSnippetsFetcher::CHROME_READER_API); - builder.fetch_api = NTPSnippetsFetcher::CHROME_READER_API; - EXPECT_THAT(builder.BuildRequest(), + EXPECT_THAT(builder.PreviewRequestHeadersForTesting(), + StrEq("Content-Type: application/json; charset=UTF-8\r\n" + "Authorization: headerstuff\r\n" + "\r\n")); + EXPECT_THAT(builder.PreviewRequestBodyForTesting(), EqualsJSON("{" " \"response_detail_level\": \"STANDARD\"," " \"obfuscated_gaia_id\": \"0BFUSGAIA\"," - " \"user_locale\": \"en\"," " \"advanced_options\": {" " \"local_scoring_params\": {" " \"content_params\": {" @@ -345,10 +398,10 @@ " }" "}")); - builder.fetch_api = NTPSnippetsFetcher::CHROME_CONTENT_SUGGESTIONS_API; - EXPECT_THAT(builder.BuildRequest(), + builder.SetFetchAPI( + NTPSnippetsFetcher::FetchAPI::CHROME_CONTENT_SUGGESTIONS_API); + EXPECT_THAT(builder.PreviewRequestBodyForTesting(), EqualsJSON("{" - " \"uiLanguage\": \"en\"," " \"priority\": \"BACKGROUND_PREFETCH\"," " \"regularlyVisitedHostNames\": [" " \"chromium.org\"" @@ -362,13 +415,17 @@ TEST_F(NTPSnippetsFetcherTest, BuildRequestUnauthenticated) { NTPSnippetsFetcher::RequestBuilder builder; - builder.params = test_params(); - builder.params.count_to_fetch = 10; - builder.only_return_personalized_results = false; - builder.user_class = "ACTIVE_NTP_USER"; + NTPSnippetsFetcher::Params params = test_params(); + params.count_to_fetch = 10; + builder.SetParams(params) + .SetUserClassForTesting("ACTIVE_NTP_USER") + .SetPersonalization(NTPSnippetsFetcher::Personalization::kNonPersonal) + .SetFetchAPI(NTPSnippetsFetcher::CHROME_READER_API); - builder.fetch_api = NTPSnippetsFetcher::CHROME_READER_API; - EXPECT_THAT(builder.BuildRequest(), + EXPECT_THAT(builder.PreviewRequestHeadersForTesting(), + StrEq("Content-Type: application/json; charset=UTF-8\r\n" + "\r\n")); + EXPECT_THAT(builder.PreviewRequestBodyForTesting(), EqualsJSON("{" " \"response_detail_level\": \"STANDARD\"," " \"advanced_options\": {" @@ -399,8 +456,9 @@ " }" "}")); - builder.fetch_api = NTPSnippetsFetcher::CHROME_CONTENT_SUGGESTIONS_API; - EXPECT_THAT(builder.BuildRequest(), + builder.SetFetchAPI( + NTPSnippetsFetcher::FetchAPI::CHROME_CONTENT_SUGGESTIONS_API); + EXPECT_THAT(builder.PreviewRequestBodyForTesting(), EqualsJSON("{" " \"regularlyVisitedHostNames\": []," " \"priority\": \"USER_ACTION\"," @@ -411,16 +469,18 @@ TEST_F(NTPSnippetsFetcherTest, BuildRequestExcludedIds) { NTPSnippetsFetcher::RequestBuilder builder; - builder.params = test_params(); - builder.params.interactive_request = false; + NTPSnippetsFetcher::Params params = test_params(); + params.interactive_request = false; for (int i = 0; i < 200; ++i) { - builder.params.excluded_ids.insert(base::StringPrintf("%03d", i)); + params.excluded_ids.insert(base::StringPrintf("%03d", i)); } - builder.only_return_personalized_results = false; - builder.user_class = "ACTIVE_NTP_USER"; + builder.SetParams(params) + .SetUserClassForTesting("ACTIVE_NTP_USER") + .SetPersonalization(NTPSnippetsFetcher::Personalization::kNonPersonal) + .SetFetchAPI( + NTPSnippetsFetcher::FetchAPI::CHROME_CONTENT_SUGGESTIONS_API); - builder.fetch_api = NTPSnippetsFetcher::CHROME_CONTENT_SUGGESTIONS_API; - EXPECT_THAT(builder.BuildRequest(), + EXPECT_THAT(builder.PreviewRequestBodyForTesting(), EqualsJSON("{" " \"regularlyVisitedHostNames\": []," " \"priority\": \"BACKGROUND_PREFETCH\"," @@ -454,12 +514,14 @@ TEST_F(NTPSnippetsFetcherTest, BuildRequestNoUserClass) { NTPSnippetsFetcher::RequestBuilder builder; - builder.params = test_params(); - builder.params.interactive_request = false; - builder.only_return_personalized_results = false; + NTPSnippetsFetcher::Params params = test_params(); + params.interactive_request = false; + builder.SetPersonalization(NTPSnippetsFetcher::Personalization::kNonPersonal) + .SetParams(params) + .SetFetchAPI( + NTPSnippetsFetcher::FetchAPI::CHROME_CONTENT_SUGGESTIONS_API); - builder.fetch_api = NTPSnippetsFetcher::CHROME_CONTENT_SUGGESTIONS_API; - EXPECT_THAT(builder.BuildRequest(), + EXPECT_THAT(builder.PreviewRequestBodyForTesting(), EqualsJSON("{" " \"regularlyVisitedHostNames\": []," " \"priority\": \"BACKGROUND_PREFETCH\"," @@ -468,19 +530,22 @@ } TEST_F(NTPSnippetsFetcherTest, BuildRequestWithTwoLanguages) { - NTPSnippetsFetcher::RequestBuilder builder; - builder.params = test_params(); - builder.only_return_personalized_results = false; - builder.ui_language.language_code = "en"; - builder.ui_language.frequency = 0.5f; - builder.other_top_language.language_code = "de"; - builder.other_top_language.frequency = 0.5f; + RequestBuilderWithMockedFlagsForTesting builder; + std::unique_ptr<translate::LanguageModel> language_model = + MakeLanguageModel({"de", "en"}); + NTPSnippetsFetcher::Params params = test_params(); + params.language_code = "en"; + builder.SetParams(params) + .SetLanguageModel(language_model.get()) + .SetPersonalization(NTPSnippetsFetcher::Personalization::kNonPersonal) + .SetFetchAPI( + NTPSnippetsFetcher::FetchAPI::CHROME_CONTENT_SUGGESTIONS_API); - builder.fetch_api = NTPSnippetsFetcher::CHROME_CONTENT_SUGGESTIONS_API; - EXPECT_THAT(builder.BuildRequest(), + EXPECT_THAT(builder.PreviewRequestBodyForTesting(), EqualsJSON("{" " \"regularlyVisitedHostNames\": []," " \"priority\": \"USER_ACTION\"," + " \"uiLanguage\": \"en\"," " \"excludedSuggestionIds\": []," " \"topLanguages\": [" " {" @@ -496,41 +561,26 @@ } TEST_F(NTPSnippetsFetcherTest, BuildRequestWithUILanguageOnly) { - NTPSnippetsFetcher::RequestBuilder builder; - builder.params = test_params(); - builder.only_return_personalized_results = false; - builder.ui_language.language_code = "en"; - builder.ui_language.frequency = 0.5f; + RequestBuilderWithMockedFlagsForTesting builder; + std::unique_ptr<translate::LanguageModel> language_model = + MakeLanguageModel({"en"}); + NTPSnippetsFetcher::Params params = test_params(); + params.language_code = "en"; + builder.SetParams(params) + .SetLanguageModel(language_model.get()) + .SetPersonalization(NTPSnippetsFetcher::Personalization::kNonPersonal) + .SetFetchAPI( + NTPSnippetsFetcher::FetchAPI::CHROME_CONTENT_SUGGESTIONS_API); - builder.fetch_api = NTPSnippetsFetcher::CHROME_CONTENT_SUGGESTIONS_API; - EXPECT_THAT(builder.BuildRequest(), + EXPECT_THAT(builder.PreviewRequestBodyForTesting(), EqualsJSON("{" " \"regularlyVisitedHostNames\": []," " \"priority\": \"USER_ACTION\"," + " \"uiLanguage\": \"en\"," " \"excludedSuggestionIds\": []," " \"topLanguages\": [{" " \"language\" : \"en\"," - " \"frequency\" : 0.5" - " }]" - "}")); -} - -TEST_F(NTPSnippetsFetcherTest, BuildRequestWithOtherLanguageOnly) { - NTPSnippetsFetcher::RequestBuilder builder; - builder.params = test_params(); - builder.only_return_personalized_results = false; - builder.other_top_language.language_code = "de"; - builder.other_top_language.frequency = 0.5f; - - builder.fetch_api = NTPSnippetsFetcher::CHROME_CONTENT_SUGGESTIONS_API; - EXPECT_THAT(builder.BuildRequest(), - EqualsJSON("{" - " \"regularlyVisitedHostNames\": []," - " \"priority\": \"USER_ACTION\"," - " \"excludedSuggestionIds\": []," - " \"topLanguages\": [{" - " \"language\" : \"de\"," - " \"frequency\" : 0.5" + " \"frequency\" : 1.0" " }]" "}")); } @@ -817,6 +867,27 @@ EXPECT_THAT(category.snippets[0]->url().spec(), Eq("http://localhost/foo2")); } +TEST_F(NTPSnippetsFetcherTest, PersonalizesDependingOnVariations) { + // Default setting should be both personalization options. + EXPECT_THAT(snippets_fetcher().personalization(), + Eq(NTPSnippetsFetcher::Personalization::kBoth)); + + SetFetchingPersonalizationVariation("personal"); + ResetSnippetsFetcher(); + EXPECT_THAT(snippets_fetcher().personalization(), + Eq(NTPSnippetsFetcher::Personalization::kPersonal)); + + SetFetchingPersonalizationVariation("non_personal"); + ResetSnippetsFetcher(); + EXPECT_THAT(snippets_fetcher().personalization(), + Eq(NTPSnippetsFetcher::Personalization::kNonPersonal)); + + SetFetchingPersonalizationVariation("both"); + ResetSnippetsFetcher(); + EXPECT_THAT(snippets_fetcher().personalization(), + Eq(NTPSnippetsFetcher::Personalization::kBoth)); +} + TEST_F(NTPSnippetsFetcherTest, ShouldFetchSuccessfullyEmptyList) { const std::string kJsonStr = "{\"recos\": []}"; SetFakeResponse(/*response_data=*/kJsonStr, net::HTTP_OK, @@ -865,6 +936,16 @@ ASSERT_TRUE(content_selectors->GetDictionary(1, &content_selector)); EXPECT_TRUE(content_selector->GetString("value", &content_selector_value)); EXPECT_THAT(content_selector_value, Eq("www.somehost2.com")); + // Call the delegate callback manually as the TestURLFetcher deletes any + // call to the delegate that usually happens on |Start|. + // Without the call to the delegate, it leaks the request that owns itself. + ASSERT_THAT(fetcher->delegate(), NotNull()); + EXPECT_CALL(mock_callback(), Run(/*snippets=*/Not(HasValue()))).Times(1); + // An 4XX response needs the least configuration to successfully invoke the + // callback properly as the results are not important in this test. + fetcher->set_response_code(net::HTTP_NOT_FOUND); + fetcher->set_status(net::URLRequestStatus(net::URLRequestStatus::FAILED, -2)); + fetcher->delegate()->OnURLFetchComplete(fetcher); } TEST_F(NTPSnippetsFetcherTest, ShouldReportUrlStatusError) { @@ -973,27 +1054,33 @@ FastForwardUntilNoTasksRemain(); } -TEST_F(NTPSnippetsFetcherTest, ShouldCancelOngoingFetch) { +TEST_F(NTPSnippetsFetcherTest, ShouldProcessConcurrentFetches) { const std::string kJsonStr = "{ \"recos\": [] }"; SetFakeResponse(/*response_data=*/kJsonStr, net::HTTP_OK, net::URLRequestStatus::SUCCESS); - EXPECT_CALL(mock_callback(), Run(IsEmptyArticleList())); + EXPECT_CALL(mock_callback(), Run(IsEmptyArticleList())).Times(5); snippets_fetcher().FetchSnippets( test_params(), ToSnippetsAvailableCallback(&mock_callback())); - // Second call to FetchSnippets() overrides/cancels the previous. - // Callback is expected to be called once. + // More calls to FetchSnippets() do not interrupt the previous. + // Callback is expected to be called once each time. + snippets_fetcher().FetchSnippets( + test_params(), ToSnippetsAvailableCallback(&mock_callback())); + snippets_fetcher().FetchSnippets( + test_params(), ToSnippetsAvailableCallback(&mock_callback())); + snippets_fetcher().FetchSnippets( + test_params(), ToSnippetsAvailableCallback(&mock_callback())); snippets_fetcher().FetchSnippets( test_params(), ToSnippetsAvailableCallback(&mock_callback())); FastForwardUntilNoTasksRemain(); EXPECT_THAT( histogram_tester().GetAllSamples("NewTabPage.Snippets.FetchResult"), - ElementsAre(base::Bucket(/*min=*/0, /*count=*/1))); + ElementsAre(base::Bucket(/*min=*/0, /*count=*/5))); EXPECT_THAT(histogram_tester().GetAllSamples( "NewTabPage.Snippets.FetchHttpResponseOrErrorCode"), - ElementsAre(base::Bucket(/*min=*/200, /*count=*/1))); + ElementsAre(base::Bucket(/*min=*/200, /*count=*/5))); EXPECT_THAT(histogram_tester().GetAllSamples("NewTabPage.Snippets.FetchTime"), ElementsAre(base::Bucket(/*min=*/kTestJsonParsingLatencyMs, - /*count=*/1))); + /*count=*/5))); } ::std::ostream& operator<<(
diff --git a/components/test/data/autofill/OWNERS b/components/test/data/autofill/OWNERS index 3883338..9d1ddcd 100644 --- a/components/test/data/autofill/OWNERS +++ b/components/test/data/autofill/OWNERS
@@ -1,5 +1,5 @@ estade@chromium.org -isherman@chromium.org +mathp@chromium.org # Owner for password autofill/generation only. gcasto@chromium.org
diff --git a/content/browser/BUILD.gn b/content/browser/BUILD.gn index 5bd8c981..94e8681b 100644 --- a/content/browser/BUILD.gn +++ b/content/browser/BUILD.gn
@@ -291,6 +291,8 @@ "appcache/appcache_working_set.h", "appcache/chrome_appcache_service.cc", "appcache/chrome_appcache_service.h", + "audio_device_thread.cc", + "audio_device_thread.h", "background_sync/background_sync_context.cc", "background_sync/background_sync_context.h", "background_sync/background_sync_manager.cc", @@ -1087,6 +1089,8 @@ "renderer_host/media/audio_input_sync_writer.h", "renderer_host/media/audio_output_authorization_handler.cc", "renderer_host/media/audio_output_authorization_handler.h", + "renderer_host/media/audio_output_delegate.cc", + "renderer_host/media/audio_output_delegate.h", "renderer_host/media/audio_renderer_host.cc", "renderer_host/media/audio_renderer_host.h", "renderer_host/media/audio_sync_reader.cc",
diff --git a/content/browser/audio_device_thread.cc b/content/browser/audio_device_thread.cc new file mode 100644 index 0000000..21cceb5 --- /dev/null +++ b/content/browser/audio_device_thread.cc
@@ -0,0 +1,28 @@ +// Copyright 2016 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "base/threading/thread_task_runner_handle.h" +#include "build/build_config.h" +#include "content/browser/audio_device_thread.h" + +namespace content { + +scoped_refptr<base::SingleThreadTaskRunner> AudioDeviceThread::GetTaskRunner() { +#if defined(OS_MACOSX) + // On Mac audio task runner must belong to the main thread. + // See http://crbug.com/158170. + return base::ThreadTaskRunnerHandle::Get(); +#else + return thread_.task_runner(); +#endif // defined(OS_MACOSX) + } + +AudioDeviceThread::AudioDeviceThread() : thread_("AudioThread") { +#if defined(OS_WIN) + thread_.init_com_with_mta(true); +#endif + CHECK(thread_.Start()); +} + +} // namespace content
diff --git a/content/browser/audio_device_thread.h b/content/browser/audio_device_thread.h new file mode 100644 index 0000000..1547686 --- /dev/null +++ b/content/browser/audio_device_thread.h
@@ -0,0 +1,36 @@ +// Copyright 2016 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef CONTENT_BROWSER_AUDIO_DEVICE_THREAD_H_ +#define CONTENT_BROWSER_AUDIO_DEVICE_THREAD_H_ + +#include "base/threading/thread.h" +#include "content/common/content_export.h" + +namespace content { + +// This class encapulates the logic for the thread and task runners that the +// AudioManager and related classes run on. audio_task_runner and +// worker_task_runner should be called on the same thread as the +// AudioDeviceThread was constructed on. +class CONTENT_EXPORT AudioDeviceThread { + public: + // Constructs and starts the thread. + AudioDeviceThread(); + + scoped_refptr<base::SingleThreadTaskRunner> GetTaskRunner(); + + scoped_refptr<base::SingleThreadTaskRunner> worker_task_runner() { + return thread_.task_runner(); + } + + private: + base::Thread thread_; + + DISALLOW_COPY_AND_ASSIGN(AudioDeviceThread); +}; + +} // namespace content + +#endif // CONTENT_BROWSER_AUDIO_DEVICE_THREAD_H_
diff --git a/content/browser/browser_main_loop.cc b/content/browser/browser_main_loop.cc index 5c5360f..14cfcee 100644 --- a/content/browser/browser_main_loop.cc +++ b/content/browser/browser_main_loop.cc
@@ -43,6 +43,7 @@ #include "components/tracing/common/process_metrics_memory_dump_provider.h" #include "components/tracing/common/trace_to_console.h" #include "components/tracing/common/tracing_switches.h" +#include "content/browser/audio_device_thread.h" #include "content/browser/browser_thread_impl.h" #include "content/browser/device_sensors/device_sensor_service.h" #include "content/browser/dom_storage/dom_storage_area.h" @@ -1606,25 +1607,10 @@ audio_manager_ = GetContentClient()->browser()->CreateAudioManager( MediaInternals::GetInstance()); if (!audio_manager_) { - audio_thread_.reset(new base::Thread("AudioThread")); -#if defined(OS_WIN) - audio_thread_->init_com_with_mta(true); -#endif // defined(OS_WIN) - CHECK(audio_thread_->Start()); -#if defined(OS_MACOSX) - // On Mac audio task runner must belong to the main thread. - // See http://crbug.com/158170. - scoped_refptr<base::SingleThreadTaskRunner> audio_task_runner = - base::ThreadTaskRunnerHandle::Get(); -#else - scoped_refptr<base::SingleThreadTaskRunner> audio_task_runner = - audio_thread_->task_runner(); -#endif // defined(OS_MACOSX) - scoped_refptr<base::SingleThreadTaskRunner> worker_task_runner = - audio_thread_->task_runner(); - audio_manager_ = media::AudioManager::Create(std::move(audio_task_runner), - std::move(worker_task_runner), - MediaInternals::GetInstance()); + audio_thread_ = base::MakeUnique<AudioDeviceThread>(); + audio_manager_ = media::AudioManager::Create( + audio_thread_->GetTaskRunner(), audio_thread_->worker_task_runner(), + MediaInternals::GetInstance()); } CHECK(audio_manager_); }
diff --git a/content/browser/browser_main_loop.h b/content/browser/browser_main_loop.h index 1464de7c..b3c357de 100644 --- a/content/browser/browser_main_loop.h +++ b/content/browser/browser_main_loop.h
@@ -72,6 +72,7 @@ #endif namespace content { +class AudioDeviceThread; class BrowserMainParts; class BrowserOnlineStateObserver; class BrowserThreadImpl; @@ -287,7 +288,7 @@ // |user_input_monitor_| has to outlive |audio_manager_|, so declared first. std::unique_ptr<media::UserInputMonitor> user_input_monitor_; // AudioThread needs to outlive |audio_manager_|. - std::unique_ptr<base::Thread> audio_thread_; + std::unique_ptr<AudioDeviceThread> audio_thread_; media::ScopedAudioManagerPtr audio_manager_; std::unique_ptr<midi::MidiManager> midi_manager_;
diff --git a/content/browser/renderer_host/media/audio_output_authorization_handler_unittest.cc b/content/browser/renderer_host/media/audio_output_authorization_handler_unittest.cc index 77edb06..9b5c54d 100644 --- a/content/browser/renderer_host/media/audio_output_authorization_handler_unittest.cc +++ b/content/browser/renderer_host/media/audio_output_authorization_handler_unittest.cc
@@ -9,11 +9,11 @@ #include "base/bind.h" #include "base/command_line.h" #include "base/run_loop.h" +#include "content/browser/audio_device_thread.h" #include "content/browser/browser_thread_impl.h" #include "content/public/browser/browser_thread.h" #include "content/public/test/mock_render_process_host.h" #include "content/public/test/test_browser_context.h" -#include "content/public/test/test_browser_thread.h" #include "content/public/test/test_browser_thread_bundle.h" #include "media/audio/audio_device_description.h" #include "media/audio/fake_audio_log_factory.h" @@ -71,23 +71,10 @@ thread_bundle_ = base::MakeUnique<TestBrowserThreadBundle>( TestBrowserThreadBundle::Options::REAL_IO_THREAD); - audio_thread_ = base::MakeUnique<base::Thread>("AudioThread"); - -// Audio manager creation stolen from content/browser/browser_main_loop.cc. -#if defined(OS_WIN) - audio_thread_->init_com_with_mta(true); -#endif // defined(OS_WIN) - CHECK(audio_thread_->Start()); - -#if defined(OS_MACOSX) - scoped_refptr<base::SingleThreadTaskRunner> audio_task_runner = - base::ThreadTaskRunnerHandle::Get(); -#else - scoped_refptr<base::SingleThreadTaskRunner> audio_task_runner = - audio_thread_->task_runner(); -#endif // defined(OS_MACOSX) + audio_thread_ = base::MakeUnique<AudioDeviceThread>(); audio_manager_.reset(new media::FakeAudioManager( - audio_task_runner, audio_thread_->task_runner(), &log_factory_)); + audio_thread_->GetTaskRunner(), audio_thread_->worker_task_runner(), + &log_factory_)); media_stream_manager_ = base::MakeUnique<MediaStreamManager>(audio_manager_.get()); // Make sure everything is done initializing: @@ -110,7 +97,7 @@ for (int i = 0; i < 20; ++i) { base::RunLoop().RunUntilIdle(); SyncWith(BrowserThread::GetTaskRunnerForThread(BrowserThread::IO)); - SyncWith(audio_thread_->task_runner()); + SyncWith(audio_manager_->GetWorkerTaskRunner()); } } @@ -159,7 +146,7 @@ // DestructionObserver. std::unique_ptr<MediaStreamManager> media_stream_manager_; std::unique_ptr<TestBrowserThreadBundle> thread_bundle_; - std::unique_ptr<base::Thread> audio_thread_; + std::unique_ptr<AudioDeviceThread> audio_thread_; media::FakeAudioLogFactory log_factory_; media::ScopedAudioManagerPtr audio_manager_;
diff --git a/content/browser/renderer_host/media/audio_output_delegate.cc b/content/browser/renderer_host/media/audio_output_delegate.cc new file mode 100644 index 0000000..32d936eb --- /dev/null +++ b/content/browser/renderer_host/media/audio_output_delegate.cc
@@ -0,0 +1,190 @@ +// Copyright 2016 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "content/browser/renderer_host/media/audio_output_delegate.h" + +#include <utility> + +#include "base/bind.h" +#include "content/browser/media/audio_stream_monitor.h" +#include "content/browser/media/capture/audio_mirroring_manager.h" +#include "content/browser/media/media_internals.h" +#include "content/browser/renderer_host/media/audio_sync_reader.h" +#include "content/public/browser/browser_thread.h" +#include "content/public/browser/media_observer.h" + +namespace content { + +AudioOutputDelegate::AudioOutputDelegate( + EventHandler* handler, + media::AudioManager* audio_manager, + std::unique_ptr<media::AudioLog> audio_log, + int stream_id, + int render_frame_id, + int render_process_id, + const media::AudioParameters& params, + const std::string& output_device_id) + : handler_(handler), + audio_log_(std::move(audio_log)), + reader_(AudioSyncReader::Create(params)), + stream_id_(stream_id), + render_frame_id_(render_frame_id), + render_process_id_(render_process_id), + weak_factory_(this) { + DCHECK(handler_); + DCHECK(audio_manager); + DCHECK(audio_log_); + weak_this_ = weak_factory_.GetWeakPtr(); + audio_log_->OnCreated(stream_id, params, output_device_id); + controller_ = media::AudioOutputController::Create( + audio_manager, this, params, output_device_id, reader_.get()); + DCHECK(controller_); +} + +AudioOutputDelegate::~AudioOutputDelegate() { + DCHECK_CURRENTLY_ON(BrowserThread::IO); + DCHECK(!playing_); + DCHECK(!handler_); +} + +void AudioOutputDelegate::Deleter::operator()(AudioOutputDelegate* delegate) { + DCHECK_CURRENTLY_ON(BrowserThread::IO); + delegate->UpdatePlayingState(false); + delegate->handler_ = nullptr; + delegate->audio_log_->OnClosed(delegate->stream_id_); + + // |controller| will call the closure (on IO thread) when it's done closing, + // and it is only after that call that we can delete |delegate|. By giving the + // closure ownership of |delegate|, we keep delegate alive until |controller| + // is closed. + // + // The mirroring manager is a leaky singleton, so Unretained is fine. + delegate->controller_->Close(base::Bind( + [](AudioOutputDelegate* delegate, + AudioMirroringManager* mirroring_manager) { + // De-register the controller from the AudioMirroringManager now that + // the controller has closed the AudioOutputStream and shut itself down. + // This ensures that calling RemoveDiverter() here won't trigger the + // controller to re-start the default AudioOutputStream and cause a + // brief audio blip to come out the user's speakers. + // http://crbug.com/474432 + // + // It's fine if the task is canceled during shutdown, since the + // mirroring manager doesn't require that all diverters are + // removed. + if (mirroring_manager) + mirroring_manager->RemoveDiverter(delegate->controller_.get()); + }, + base::Owned(delegate), base::Unretained(mirroring_manager_))); +} + +// static +AudioOutputDelegate::UniquePtr AudioOutputDelegate::Create( + EventHandler* handler, + media::AudioManager* audio_manager, + std::unique_ptr<media::AudioLog> audio_log, + AudioMirroringManager* mirroring_manager, + MediaObserver* media_observer, + int stream_id, + int render_frame_id, + int render_process_id, + const media::AudioParameters& params, + const std::string& output_device_id) { + if (media_observer) + media_observer->OnCreatingAudioStream(render_process_id, render_frame_id); + UniquePtr delegate( + new AudioOutputDelegate(handler, audio_manager, std::move(audio_log), + stream_id, render_frame_id, render_process_id, + params, output_device_id), + Deleter(mirroring_manager)); + if (mirroring_manager) + mirroring_manager->AddDiverter(render_process_id, render_frame_id, + delegate->controller_.get()); + return delegate; +} + +void AudioOutputDelegate::OnPlayStream() { + DCHECK_CURRENTLY_ON(BrowserThread::IO); + controller_->Play(); + audio_log_->OnStarted(stream_id_); +} + +void AudioOutputDelegate::OnPauseStream() { + DCHECK_CURRENTLY_ON(BrowserThread::IO); + controller_->Pause(); + audio_log_->OnStopped(stream_id_); +} + +void AudioOutputDelegate::OnSetVolume(double volume) { + DCHECK_CURRENTLY_ON(BrowserThread::IO); + DCHECK_GE(volume, 0); + DCHECK_LE(volume, 1); + controller_->SetVolume(volume); + audio_log_->OnSetVolume(stream_id_, volume); +} + +void AudioOutputDelegate::OnControllerCreated() { + BrowserThread::PostTask( + BrowserThread::IO, FROM_HERE, + base::Bind(&AudioOutputDelegate::SendCreatedNotification, weak_this_)); +} + +void AudioOutputDelegate::OnControllerPlaying() { + BrowserThread::PostTask( + BrowserThread::IO, FROM_HERE, + base::Bind(&AudioOutputDelegate::UpdatePlayingState, weak_this_, true)); +} + +void AudioOutputDelegate::OnControllerPaused() { + BrowserThread::PostTask( + BrowserThread::IO, FROM_HERE, + base::Bind(&AudioOutputDelegate::UpdatePlayingState, weak_this_, false)); +} + +void AudioOutputDelegate::OnControllerError() { + BrowserThread::PostTask( + BrowserThread::IO, FROM_HERE, + base::Bind(&AudioOutputDelegate::OnError, weak_this_)); +} + +void AudioOutputDelegate::SendCreatedNotification() { + DCHECK_CURRENTLY_ON(BrowserThread::IO); + if (handler_) { + handler_->OnStreamCreated(stream_id_, reader_->shared_memory(), + reader_->foreign_socket()); + } +} + +void AudioOutputDelegate::UpdatePlayingState(bool playing) { + DCHECK_CURRENTLY_ON(BrowserThread::IO); + if (!handler_ || playing == playing_) + return; + + playing_ = playing; + handler_->OnStreamStateChanged(playing); + if (playing) { + // Note that this takes a reference to |controller_|, and + // (Start|Stop)MonitoringStream calls are async, so we don't have a + // guarantee for when the controller is destroyed. + AudioStreamMonitor::StartMonitoringStream( + render_process_id_, render_frame_id_, stream_id_, + base::Bind(&media::AudioOutputController::ReadCurrentPowerAndClip, + controller_)); + } else { + AudioStreamMonitor::StopMonitoringStream(render_process_id_, + render_frame_id_, stream_id_); + } +} + +void AudioOutputDelegate::OnError() { + DCHECK_CURRENTLY_ON(BrowserThread::IO); + + if (!handler_) + return; + + audio_log_->OnError(stream_id_); + handler_->OnStreamError(stream_id_); +} + +} // namespace content
diff --git a/content/browser/renderer_host/media/audio_output_delegate.h b/content/browser/renderer_host/media/audio_output_delegate.h new file mode 100644 index 0000000..2744295a --- /dev/null +++ b/content/browser/renderer_host/media/audio_output_delegate.h
@@ -0,0 +1,145 @@ +// Copyright 2016 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef CONTENT_BROWSER_RENDERER_HOST_MEDIA_AUDIO_OUTPUT_DELEGATE_H_ +#define CONTENT_BROWSER_RENDERER_HOST_MEDIA_AUDIO_OUTPUT_DELEGATE_H_ + +#include <memory> +#include <string> + +#include "base/macros.h" +#include "base/memory/weak_ptr.h" +#include "content/common/content_export.h" +#include "media/audio/audio_output_controller.h" + +namespace base { +class SharedMemory; +class CancelableSyncSocket; +} + +namespace content { +class AudioMirroringManager; +class AudioSyncReader; +class MediaObserver; +} + +namespace media { +class AudioLog; +class AudioParameters; +} + +namespace content { + +// This class, except for the AudioOutputDelegate::EventHandler implementation, +// is operated on the IO thread. +class CONTENT_EXPORT AudioOutputDelegate + : public media::AudioOutputController::EventHandler { + public: + class CONTENT_EXPORT EventHandler { + public: + virtual ~EventHandler() {} + + // All these methods are called on the IO thread. + + // Called when the state changes between playing and not playing. + virtual void OnStreamStateChanged(bool playing) = 0; + + // Called when construction is finished and the stream is ready for + // playout. + virtual void OnStreamCreated(int stream_id, + base::SharedMemory* shared_memory, + base::CancelableSyncSocket* socket) = 0; + + // Called if stream encounters an error and has become unusable. + virtual void OnStreamError(int stream_id) = 0; + }; + + class CONTENT_EXPORT Deleter { + public: + Deleter() = default; + explicit Deleter(AudioMirroringManager* mirroring_manager) + : mirroring_manager_(mirroring_manager) {} + void operator()(AudioOutputDelegate* delegate); + + private: + AudioMirroringManager* mirroring_manager_; + }; + + using UniquePtr = std::unique_ptr<AudioOutputDelegate, Deleter>; + + // The AudioOutputDelegate might issue callbacks until the UniquePtr + // destructor finishes, so calling |handler| must be valid until then. + static UniquePtr Create(EventHandler* handler, + media::AudioManager* audio_manager, + std::unique_ptr<media::AudioLog> audio_log, + AudioMirroringManager* mirroring_manager, + MediaObserver* media_observer, + int stream_id, + int render_frame_id, + int render_process_id, + const media::AudioParameters& params, + const std::string& output_device_id); + ~AudioOutputDelegate() override; + + // TODO(maxmorin): Remove this when crbug.com/647185 is closed. + // This function is used to provide control of the audio stream to + // WebrtcAudioPrivateGetActiveSinkFunction and others in the webrtc extension + // API. Since the controller is shared, this means that it might outlive the + // AudioOutputDelegate. In this case, it is still safe to call functions on + // the controller, but it will not do anything. The controller is also shared + // with AudioStreamMonitor. + scoped_refptr<media::AudioOutputController> controller() const { + return controller_; + } + + int stream_id() const { return stream_id_; } + + // Stream control: + void OnPlayStream(); + void OnPauseStream(); + void OnSetVolume(double volume); + + // AudioOutputController::EventHandler implementation. These methods may + // be called on any thead. + void OnControllerCreated() override; + void OnControllerPlaying() override; + void OnControllerPaused() override; + void OnControllerError() override; + + private: + AudioOutputDelegate(EventHandler* handler, + media::AudioManager* audio_manager, + std::unique_ptr<media::AudioLog> audio_log, + int stream_id, + int render_frame_id, + int render_process_id, + const media::AudioParameters& params, + const std::string& output_device_id); + + void SendCreatedNotification(); + void OnError(); + void UpdatePlayingState(bool playing); + + // |handler_| is null if we are in the process of destruction. In this case, + // we will ignore events from |controller_|. + EventHandler* handler_; + std::unique_ptr<media::AudioLog> const audio_log_; + std::unique_ptr<AudioSyncReader> reader_; + scoped_refptr<media::AudioOutputController> controller_; + const int stream_id_; + const int render_frame_id_; + const int render_process_id_; + + // This flag ensures that we only send OnStreamStateChanged notifications + // and (de)register with the stream monitor when the state actually changes. + bool playing_ = false; + base::WeakPtr<AudioOutputDelegate> weak_this_; + base::WeakPtrFactory<AudioOutputDelegate> weak_factory_; + + DISALLOW_COPY_AND_ASSIGN(AudioOutputDelegate); +}; + +} // namespace content + +#endif // CONTENT_BROWSER_RENDERER_HOST_MEDIA_AUDIO_OUTPUT_DELEGATE_H_
diff --git a/content/browser/renderer_host/media/audio_output_delegate_unittest.cc b/content/browser/renderer_host/media/audio_output_delegate_unittest.cc new file mode 100644 index 0000000..87462a1b8 --- /dev/null +++ b/content/browser/renderer_host/media/audio_output_delegate_unittest.cc
@@ -0,0 +1,589 @@ +// Copyright 2016 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "content/browser/renderer_host/media/audio_output_delegate.h" + +#include <stdint.h> + +#include <memory> +#include <utility> + +#include "base/bind.h" +#include "base/command_line.h" +#include "base/message_loop/message_loop.h" +#include "base/run_loop.h" +#include "content/browser/audio_device_thread.h" +#include "content/browser/media/capture/audio_mirroring_manager.h" +#include "content/browser/renderer_host/media/media_stream_manager.h" +#include "content/public/browser/browser_thread.h" +#include "content/public/browser/media_observer.h" +#include "content/public/test/test_browser_thread_bundle.h" +#include "media/audio/fake_audio_log_factory.h" +#include "media/audio/fake_audio_manager.h" +#include "media/base/media_switches.h" +#include "testing/gmock/include/gmock/gmock.h" +#include "testing/gtest/include/gtest/gtest.h" + +using ::testing::_; +using ::testing::InSequence; +using ::testing::NotNull; +using ::testing::StrictMock; + +// TODO(maxmorin): not yet tested: +// - Interactions with AudioStreamMonitor (goes through WebContentsImpl, +// so it's a bit tricky). +// - Logging (small risk of bugs, not worth the effort). +// - That the returned socket/memory is correctly set up. + +namespace content { + +namespace { + +const int kRenderProcessId = 1; +const int kRenderFrameId = 5; +const int kStreamId = 50; +const char kDefaultDeviceId[] = ""; + +class MockAudioMirroringManager : public AudioMirroringManager { + public: + MOCK_METHOD3(AddDiverter, + void(int render_process_id, + int render_frame_id, + Diverter* diverter)); + MOCK_METHOD1(RemoveDiverter, void(Diverter* diverter)); +}; + +class MockObserver : public content::MediaObserver { + public: + void OnAudioCaptureDevicesChanged() override {} + void OnVideoCaptureDevicesChanged() override {} + void OnMediaRequestStateChanged(int render_process_id, + int render_frame_id, + int page_request_id, + const GURL& security_origin, + MediaStreamType stream_type, + MediaRequestState state) override {} + void OnSetCapturingLinkSecured(int render_process_id, + int render_frame_id, + int page_request_id, + MediaStreamType stream_type, + bool is_secure) override {} + + MOCK_METHOD2(OnCreatingAudioStream, + void(int render_process_id, int render_frame_id)); +}; + +class MockEventHandler : public AudioOutputDelegate::EventHandler { + public: + MOCK_METHOD1(OnStreamStateChanged, void(bool playing)); + MOCK_METHOD3(OnStreamCreated, + void(int stream_id, + base::SharedMemory* shared_memory, + base::CancelableSyncSocket* socket)); + MOCK_METHOD1(OnStreamError, void(int stream_id)); +}; + +class DummyAudioOutputStream : public media::AudioOutputStream { + public: + // AudioOutputSteam implementation: + bool Open() override { return true; } + void Start(AudioSourceCallback* cb) override {} + void Stop() override {} + void SetVolume(double volume) override {} + void GetVolume(double* volume) override { *volume = 1; } + void Close() override {} +}; + +} // namespace + +class AudioOutputDelegateTest : public testing::Test { + public: + AudioOutputDelegateTest() { + base::CommandLine::ForCurrentProcess()->AppendSwitch( + switches::kUseFakeDeviceForMediaStream); + + // This test uses real UI, IO and audio threads. + // AudioOutputDelegate mainly interacts with the IO and audio threads, + // but interacts with UI for bad messages, so using these threads should + // approximate the real conditions of AudioOutputDelegate well. + thread_bundle_ = base::MakeUnique<TestBrowserThreadBundle>( + TestBrowserThreadBundle::Options::REAL_IO_THREAD); + audio_thread_ = base::MakeUnique<AudioDeviceThread>(); + + audio_manager_.reset(new media::FakeAudioManager( + audio_thread_->GetTaskRunner(), audio_thread_->worker_task_runner(), + &log_factory_)); + media_stream_manager_ = + base::MakeUnique<MediaStreamManager>(audio_manager_.get()); + } + + // Test bodies are here, so that we can run them on the IO thread. + void CreateTest(base::Closure done) { + EXPECT_CALL(media_observer_, + OnCreatingAudioStream(kRenderProcessId, kRenderFrameId)); + EXPECT_CALL(event_handler_, + OnStreamCreated(kStreamId, NotNull(), NotNull())); + EXPECT_CALL(mirroring_manager_, + AddDiverter(kRenderProcessId, kRenderFrameId, NotNull())); + + AudioOutputDelegate::UniquePtr delegate = AudioOutputDelegate::Create( + &event_handler_, audio_manager_.get(), + log_factory_.CreateAudioLog( + media::AudioLogFactory::AUDIO_OUTPUT_CONTROLLER), + &mirroring_manager_, &media_observer_, kStreamId, kRenderFrameId, + kRenderProcessId, audio_manager_->GetDefaultOutputStreamParameters(), + kDefaultDeviceId); + + SyncWithAllThreads(); + + EXPECT_CALL(mirroring_manager_, RemoveDiverter(NotNull())); + delegate.reset(); + SyncWithAllThreads(); + BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, done); + } + + void PlayTest(base::Closure done) { + EXPECT_CALL(media_observer_, + OnCreatingAudioStream(kRenderProcessId, kRenderFrameId)); + EXPECT_CALL(event_handler_, + OnStreamCreated(kStreamId, NotNull(), NotNull())); + EXPECT_CALL(mirroring_manager_, + AddDiverter(kRenderProcessId, kRenderFrameId, NotNull())); + { + InSequence s; + EXPECT_CALL(event_handler_, OnStreamStateChanged(true)); + EXPECT_CALL(event_handler_, OnStreamStateChanged(false)); + } + + AudioOutputDelegate::UniquePtr delegate = AudioOutputDelegate::Create( + &event_handler_, audio_manager_.get(), + log_factory_.CreateAudioLog( + media::AudioLogFactory::AUDIO_OUTPUT_CONTROLLER), + &mirroring_manager_, &media_observer_, kStreamId, kRenderFrameId, + kRenderProcessId, audio_manager_->GetDefaultOutputStreamParameters(), + kDefaultDeviceId); + + delegate->OnPlayStream(); + + SyncWithAllThreads(); + + EXPECT_CALL(mirroring_manager_, RemoveDiverter(NotNull())); + delegate.reset(); + SyncWithAllThreads(); + BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, done); + } + + void PauseTest(base::Closure done) { + EXPECT_CALL(media_observer_, + OnCreatingAudioStream(kRenderProcessId, kRenderFrameId)); + EXPECT_CALL(event_handler_, + OnStreamCreated(kStreamId, NotNull(), NotNull())); + EXPECT_CALL(mirroring_manager_, + AddDiverter(kRenderProcessId, kRenderFrameId, NotNull())); + + AudioOutputDelegate::UniquePtr delegate = AudioOutputDelegate::Create( + &event_handler_, audio_manager_.get(), + log_factory_.CreateAudioLog( + media::AudioLogFactory::AUDIO_OUTPUT_CONTROLLER), + &mirroring_manager_, &media_observer_, kStreamId, kRenderFrameId, + kRenderProcessId, audio_manager_->GetDefaultOutputStreamParameters(), + kDefaultDeviceId); + + delegate->OnPauseStream(); + + SyncWithAllThreads(); + + EXPECT_CALL(mirroring_manager_, RemoveDiverter(NotNull())); + delegate.reset(); + SyncWithAllThreads(); + BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, done); + } + + void PlayPausePlayTest(base::Closure done) { + EXPECT_CALL(media_observer_, + OnCreatingAudioStream(kRenderProcessId, kRenderFrameId)); + EXPECT_CALL(event_handler_, + OnStreamCreated(kStreamId, NotNull(), NotNull())); + EXPECT_CALL(mirroring_manager_, + AddDiverter(kRenderProcessId, kRenderFrameId, NotNull())); + { + InSequence s; + EXPECT_CALL(event_handler_, OnStreamStateChanged(true)); + EXPECT_CALL(event_handler_, OnStreamStateChanged(false)); + EXPECT_CALL(event_handler_, OnStreamStateChanged(true)); + EXPECT_CALL(event_handler_, OnStreamStateChanged(false)); + } + + AudioOutputDelegate::UniquePtr delegate = AudioOutputDelegate::Create( + &event_handler_, audio_manager_.get(), + log_factory_.CreateAudioLog( + media::AudioLogFactory::AUDIO_OUTPUT_CONTROLLER), + &mirroring_manager_, &media_observer_, kStreamId, kRenderFrameId, + kRenderProcessId, audio_manager_->GetDefaultOutputStreamParameters(), + kDefaultDeviceId); + + delegate->OnPlayStream(); + delegate->OnPauseStream(); + delegate->OnPlayStream(); + + SyncWithAllThreads(); + + EXPECT_CALL(mirroring_manager_, RemoveDiverter(NotNull())); + delegate.reset(); + SyncWithAllThreads(); + BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, done); + } + + void PlayPlayTest(base::Closure done) { + EXPECT_CALL(media_observer_, + OnCreatingAudioStream(kRenderProcessId, kRenderFrameId)); + EXPECT_CALL(event_handler_, + OnStreamCreated(kStreamId, NotNull(), NotNull())); + EXPECT_CALL(mirroring_manager_, + AddDiverter(kRenderProcessId, kRenderFrameId, NotNull())); + { + InSequence s; + EXPECT_CALL(event_handler_, OnStreamStateChanged(true)); + EXPECT_CALL(event_handler_, OnStreamStateChanged(false)); + } + + AudioOutputDelegate::UniquePtr delegate = AudioOutputDelegate::Create( + &event_handler_, audio_manager_.get(), + log_factory_.CreateAudioLog( + media::AudioLogFactory::AUDIO_OUTPUT_CONTROLLER), + &mirroring_manager_, &media_observer_, kStreamId, kRenderFrameId, + kRenderProcessId, audio_manager_->GetDefaultOutputStreamParameters(), + kDefaultDeviceId); + + delegate->OnPlayStream(); + delegate->OnPlayStream(); + + SyncWithAllThreads(); + + EXPECT_CALL(mirroring_manager_, RemoveDiverter(NotNull())); + delegate.reset(); + SyncWithAllThreads(); + BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, done); + } + + void CreateDivertTest(base::Closure done) { + EXPECT_CALL(media_observer_, + OnCreatingAudioStream(kRenderProcessId, kRenderFrameId)); + EXPECT_CALL(event_handler_, + OnStreamCreated(kStreamId, NotNull(), NotNull())); + EXPECT_CALL(mirroring_manager_, + AddDiverter(kRenderProcessId, kRenderFrameId, NotNull())); + + AudioOutputDelegate::UniquePtr delegate = AudioOutputDelegate::Create( + &event_handler_, audio_manager_.get(), + log_factory_.CreateAudioLog( + media::AudioLogFactory::AUDIO_OUTPUT_CONTROLLER), + &mirroring_manager_, &media_observer_, kStreamId, kRenderFrameId, + kRenderProcessId, audio_manager_->GetDefaultOutputStreamParameters(), + kDefaultDeviceId); + + DummyAudioOutputStream stream; + delegate->controller()->StartDiverting(&stream); + + SyncWithAllThreads(); + + EXPECT_CALL(mirroring_manager_, RemoveDiverter(NotNull())); + delegate.reset(); + SyncWithAllThreads(); + BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, done); + } + + void CreateDivertPauseTest(base::Closure done) { + EXPECT_CALL(media_observer_, + OnCreatingAudioStream(kRenderProcessId, kRenderFrameId)); + EXPECT_CALL(event_handler_, + OnStreamCreated(kStreamId, NotNull(), NotNull())); + EXPECT_CALL(mirroring_manager_, + AddDiverter(kRenderProcessId, kRenderFrameId, NotNull())); + + AudioOutputDelegate::UniquePtr delegate = AudioOutputDelegate::Create( + &event_handler_, audio_manager_.get(), + log_factory_.CreateAudioLog( + media::AudioLogFactory::AUDIO_OUTPUT_CONTROLLER), + &mirroring_manager_, &media_observer_, kStreamId, kRenderFrameId, + kRenderProcessId, audio_manager_->GetDefaultOutputStreamParameters(), + kDefaultDeviceId); + + DummyAudioOutputStream stream; + delegate->controller()->StartDiverting(&stream); + + SyncWithAllThreads(); + delegate->OnPauseStream(); + + SyncWithAllThreads(); + + EXPECT_CALL(mirroring_manager_, RemoveDiverter(NotNull())); + delegate.reset(); + SyncWithAllThreads(); + BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, done); + } + + void PlayDivertTest(base::Closure done) { + EXPECT_CALL(media_observer_, + OnCreatingAudioStream(kRenderProcessId, kRenderFrameId)); + EXPECT_CALL(event_handler_, + OnStreamCreated(kStreamId, NotNull(), NotNull())); + EXPECT_CALL(mirroring_manager_, + AddDiverter(kRenderProcessId, kRenderFrameId, NotNull())); + { + InSequence s; + EXPECT_CALL(event_handler_, OnStreamStateChanged(true)); + EXPECT_CALL(event_handler_, OnStreamStateChanged(false)); + } + + AudioOutputDelegate::UniquePtr delegate = AudioOutputDelegate::Create( + &event_handler_, audio_manager_.get(), + log_factory_.CreateAudioLog( + media::AudioLogFactory::AUDIO_OUTPUT_CONTROLLER), + &mirroring_manager_, &media_observer_, kStreamId, kRenderFrameId, + kRenderProcessId, audio_manager_->GetDefaultOutputStreamParameters(), + kDefaultDeviceId); + + DummyAudioOutputStream stream; + delegate->OnPlayStream(); + delegate->controller()->StartDiverting(&stream); + + SyncWithAllThreads(); + + EXPECT_CALL(mirroring_manager_, RemoveDiverter(NotNull())); + delegate.reset(); + SyncWithAllThreads(); + BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, done); + } + + void ErrorTest(base::Closure done) { + EXPECT_CALL(media_observer_, + OnCreatingAudioStream(kRenderProcessId, kRenderFrameId)); + EXPECT_CALL(event_handler_, + OnStreamCreated(kStreamId, NotNull(), NotNull())); + EXPECT_CALL(event_handler_, OnStreamError(kStreamId)); + EXPECT_CALL(mirroring_manager_, + AddDiverter(kRenderProcessId, kRenderFrameId, NotNull())); + + AudioOutputDelegate::UniquePtr delegate = AudioOutputDelegate::Create( + &event_handler_, audio_manager_.get(), + log_factory_.CreateAudioLog( + media::AudioLogFactory::AUDIO_OUTPUT_CONTROLLER), + &mirroring_manager_, &media_observer_, kStreamId, kRenderFrameId, + kRenderProcessId, audio_manager_->GetDefaultOutputStreamParameters(), + kDefaultDeviceId); + + delegate->controller()->OnError(nullptr); + + SyncWithAllThreads(); + + EXPECT_CALL(mirroring_manager_, RemoveDiverter(NotNull())); + delegate.reset(); + SyncWithAllThreads(); + BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, done); + } + + void CreateAndDestroyTest(base::Closure done) { + EXPECT_CALL(media_observer_, + OnCreatingAudioStream(kRenderProcessId, kRenderFrameId)); + EXPECT_CALL(mirroring_manager_, + AddDiverter(kRenderProcessId, kRenderFrameId, NotNull())); + EXPECT_CALL(mirroring_manager_, RemoveDiverter(NotNull())); + + AudioOutputDelegate::UniquePtr delegate = AudioOutputDelegate::Create( + &event_handler_, audio_manager_.get(), + log_factory_.CreateAudioLog( + media::AudioLogFactory::AUDIO_OUTPUT_CONTROLLER), + &mirroring_manager_, &media_observer_, kStreamId, kRenderFrameId, + kRenderProcessId, audio_manager_->GetDefaultOutputStreamParameters(), + kDefaultDeviceId); + + delegate.reset(); + SyncWithAllThreads(); + BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, done); + } + + void PlayAndDestroyTest(base::Closure done) { + EXPECT_CALL(media_observer_, + OnCreatingAudioStream(kRenderProcessId, kRenderFrameId)); + EXPECT_CALL(event_handler_, + OnStreamCreated(kStreamId, NotNull(), NotNull())); + EXPECT_CALL(mirroring_manager_, + AddDiverter(kRenderProcessId, kRenderFrameId, NotNull())); + EXPECT_CALL(mirroring_manager_, RemoveDiverter(NotNull())); + + AudioOutputDelegate::UniquePtr delegate = AudioOutputDelegate::Create( + &event_handler_, audio_manager_.get(), + log_factory_.CreateAudioLog( + media::AudioLogFactory::AUDIO_OUTPUT_CONTROLLER), + &mirroring_manager_, &media_observer_, kStreamId, kRenderFrameId, + kRenderProcessId, audio_manager_->GetDefaultOutputStreamParameters(), + kDefaultDeviceId); + + SyncWithAllThreads(); + + delegate->OnPlayStream(); + delegate.reset(); + SyncWithAllThreads(); + BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, done); + } + + void ErrorAndDestroyTest(base::Closure done) { + EXPECT_CALL(media_observer_, + OnCreatingAudioStream(kRenderProcessId, kRenderFrameId)); + EXPECT_CALL(event_handler_, + OnStreamCreated(kStreamId, NotNull(), NotNull())); + EXPECT_CALL(mirroring_manager_, + AddDiverter(kRenderProcessId, kRenderFrameId, NotNull())); + EXPECT_CALL(mirroring_manager_, RemoveDiverter(NotNull())); + + AudioOutputDelegate::UniquePtr delegate = AudioOutputDelegate::Create( + &event_handler_, audio_manager_.get(), + log_factory_.CreateAudioLog( + media::AudioLogFactory::AUDIO_OUTPUT_CONTROLLER), + &mirroring_manager_, &media_observer_, kStreamId, kRenderFrameId, + kRenderProcessId, audio_manager_->GetDefaultOutputStreamParameters(), + kDefaultDeviceId); + SyncWithAllThreads(); + + delegate->controller()->OnError(nullptr); + delegate.reset(); + SyncWithAllThreads(); + BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, done); + } + + protected: + // MediaStreamManager uses a DestructionObserver, so it must outlive the + // TestBrowserThreadBundle. + std::unique_ptr<MediaStreamManager> media_stream_manager_; + std::unique_ptr<TestBrowserThreadBundle> thread_bundle_; + std::unique_ptr<AudioDeviceThread> audio_thread_; + media::ScopedAudioManagerPtr audio_manager_; + StrictMock<MockAudioMirroringManager> mirroring_manager_; + StrictMock<MockEventHandler> event_handler_; + StrictMock<MockObserver> media_observer_; + media::FakeAudioLogFactory log_factory_; + + private: + void SyncWithAllThreads() { + DCHECK_CURRENTLY_ON(BrowserThread::IO); + // New tasks might be posted while we are syncing, but in every iteration at + // least one task will be run. 20 iterations should be enough for our code. + for (int i = 0; i < 20; ++i) { + { + base::MessageLoop::ScopedNestableTaskAllower allower( + base::MessageLoop::current()); + base::RunLoop().RunUntilIdle(); + } + SyncWith(BrowserThread::GetTaskRunnerForThread(BrowserThread::UI)); + SyncWith(audio_manager_->GetWorkerTaskRunner()); + } + } + + void SyncWith(scoped_refptr<base::SingleThreadTaskRunner> task_runner) { + CHECK(task_runner); + CHECK(!task_runner->BelongsToCurrentThread()); + base::WaitableEvent e = {base::WaitableEvent::ResetPolicy::MANUAL, + base::WaitableEvent::InitialState::NOT_SIGNALED}; + task_runner->PostTask(FROM_HERE, base::Bind(&base::WaitableEvent::Signal, + base::Unretained(&e))); + e.Wait(); + } + + DISALLOW_COPY_AND_ASSIGN(AudioOutputDelegateTest); +}; + +TEST_F(AudioOutputDelegateTest, Create) { + base::RunLoop l; + BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, + base::Bind(&AudioOutputDelegateTest::CreateTest, + base::Unretained(this), l.QuitClosure())); + l.Run(); +} + +TEST_F(AudioOutputDelegateTest, Play) { + base::RunLoop l; + BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, + base::Bind(&AudioOutputDelegateTest::PlayTest, + base::Unretained(this), l.QuitClosure())); + l.Run(); +} + +TEST_F(AudioOutputDelegateTest, Pause) { + base::RunLoop l; + BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, + base::Bind(&AudioOutputDelegateTest::PauseTest, + base::Unretained(this), l.QuitClosure())); + l.Run(); +} + +TEST_F(AudioOutputDelegateTest, PlayPausePlay) { + base::RunLoop l; + BrowserThread::PostTask( + BrowserThread::IO, FROM_HERE, + base::Bind(&AudioOutputDelegateTest::PlayPausePlayTest, + base::Unretained(this), l.QuitClosure())); + l.Run(); +} + +TEST_F(AudioOutputDelegateTest, PlayPlay) { + base::RunLoop l; + BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, + base::Bind(&AudioOutputDelegateTest::PlayPlayTest, + base::Unretained(this), l.QuitClosure())); + l.Run(); +} + +TEST_F(AudioOutputDelegateTest, PlayDivert) { + base::RunLoop l; + BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, + base::Bind(&AudioOutputDelegateTest::PlayDivertTest, + base::Unretained(this), l.QuitClosure())); + l.Run(); +} + +TEST_F(AudioOutputDelegateTest, CreateDivert) { + base::RunLoop l; + BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, + base::Bind(&AudioOutputDelegateTest::CreateDivertTest, + base::Unretained(this), l.QuitClosure())); + l.Run(); +} + +TEST_F(AudioOutputDelegateTest, Error) { + base::RunLoop l; + BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, + base::Bind(&AudioOutputDelegateTest::ErrorTest, + base::Unretained(this), l.QuitClosure())); + l.Run(); +} + +TEST_F(AudioOutputDelegateTest, CreateAndDestroy) { + base::RunLoop l; + BrowserThread::PostTask( + BrowserThread::IO, FROM_HERE, + base::Bind(&AudioOutputDelegateTest::CreateAndDestroyTest, + base::Unretained(this), l.QuitClosure())); + l.Run(); +} + +TEST_F(AudioOutputDelegateTest, PlayAndDestroy) { + base::RunLoop l; + BrowserThread::PostTask( + BrowserThread::IO, FROM_HERE, + base::Bind(&AudioOutputDelegateTest::PlayAndDestroyTest, + base::Unretained(this), l.QuitClosure())); + l.Run(); +} + +TEST_F(AudioOutputDelegateTest, ErrorAndDestroy) { + base::RunLoop l; + BrowserThread::PostTask( + BrowserThread::IO, FROM_HERE, + base::Bind(&AudioOutputDelegateTest::PlayAndDestroyTest, + base::Unretained(this), l.QuitClosure())); + l.Run(); +} + +} // namespace content
diff --git a/content/browser/renderer_host/media/audio_renderer_host.cc b/content/browser/renderer_host/media/audio_renderer_host.cc index 98e931b..2057fa8 100644 --- a/content/browser/renderer_host/media/audio_renderer_host.cc +++ b/content/browser/renderer_host/media/audio_renderer_host.cc
@@ -5,13 +5,10 @@ #include "content/browser/renderer_host/media/audio_renderer_host.h" #include <stdint.h> -#include <utility> #include "base/bind.h" #include "base/bind_helpers.h" #include "base/lazy_instance.h" -#include "base/memory/ptr_util.h" -#include "base/memory/shared_memory.h" #include "base/metrics/histogram_macros.h" #include "content/browser/bad_message.h" #include "content/browser/browser_main_loop.h" @@ -21,7 +18,6 @@ #include "content/browser/renderer_host/media/audio_input_device_manager.h" #include "content/browser/renderer_host/media/audio_sync_reader.h" #include "content/browser/renderer_host/media/media_stream_manager.h" -#include "content/browser/renderer_host/media/media_stream_ui_proxy.h" #include "content/common/media/audio_messages.h" #include "content/public/browser/content_browser_client.h" #include "content/public/browser/media_device_id.h" @@ -75,117 +71,19 @@ } // namespace -class AudioRendererHost::AudioEntry - : public media::AudioOutputController::EventHandler { - public: - ~AudioEntry() override; - - // Returns nullptr on failure. - static std::unique_ptr<AudioRendererHost::AudioEntry> Create( - AudioRendererHost* host, - int stream_id, - int render_frame_id, - const media::AudioParameters& params, - const std::string& output_device_id); - - int stream_id() const { - return stream_id_; - } - - int render_frame_id() const { return render_frame_id_; } - - media::AudioOutputController* controller() const { return controller_.get(); } - - AudioSyncReader* reader() const { return reader_.get(); } - - // Used by ARH to track the number of active streams for UMA stats. - bool playing() const { return playing_; } - void set_playing(bool playing) { playing_ = playing; } - - private: - AudioEntry(AudioRendererHost* host, - int stream_id, - int render_frame_id, - const media::AudioParameters& params, - const std::string& output_device_id, - std::unique_ptr<AudioSyncReader> reader); - - // media::AudioOutputController::EventHandler implementation. - void OnCreated() override; - void OnPlaying() override; - void OnPaused() override; - void OnError() override; - - AudioRendererHost* const host_; - const int stream_id_; - - // The routing ID of the source RenderFrame. - const int render_frame_id_; - - // The synchronous reader to be used by |controller_|. - const std::unique_ptr<AudioSyncReader> reader_; - - // The AudioOutputController that manages the audio stream. - const scoped_refptr<media::AudioOutputController> controller_; - - bool playing_; - - DISALLOW_COPY_AND_ASSIGN(AudioEntry); -}; - -AudioRendererHost::AudioEntry::AudioEntry( - AudioRendererHost* host, - int stream_id, - int render_frame_id, - const media::AudioParameters& params, - const std::string& output_device_id, - std::unique_ptr<AudioSyncReader> reader) - : host_(host), - stream_id_(stream_id), - render_frame_id_(render_frame_id), - reader_(std::move(reader)), - controller_(media::AudioOutputController::Create(host->audio_manager_, - this, - params, - output_device_id, - reader_.get())), - playing_(false) { - DCHECK(controller_); -} - -AudioRendererHost::AudioEntry::~AudioEntry() {} - -// static -std::unique_ptr<AudioRendererHost::AudioEntry> -AudioRendererHost::AudioEntry::Create(AudioRendererHost* host, - int stream_id, - int render_frame_id, - const media::AudioParameters& params, - const std::string& output_device_id) { - std::unique_ptr<AudioSyncReader> reader(AudioSyncReader::Create(params)); - if (!reader) { - return nullptr; - } - return base::WrapUnique(new AudioEntry(host, stream_id, render_frame_id, - params, output_device_id, - std::move(reader))); -} - /////////////////////////////////////////////////////////////////////////////// // AudioRendererHost implementations. AudioRendererHost::AudioRendererHost(int render_process_id, media::AudioManager* audio_manager, AudioMirroringManager* mirroring_manager, - MediaInternals* media_internals, MediaStreamManager* media_stream_manager, const std::string& salt) : BrowserMessageFilter(AudioMsgStart), render_process_id_(render_process_id), audio_manager_(audio_manager), mirroring_manager_(mirroring_manager), - audio_log_(media_internals->CreateAudioLog( - media::AudioLogFactory::AUDIO_OUTPUT_CONTROLLER)), + media_stream_manager_(media_stream_manager), num_playing_streams_(0), salt_(salt), validate_render_frame_id_function_(&ValidateRenderFrameId), @@ -199,7 +97,7 @@ AudioRendererHost::~AudioRendererHost() { DCHECK_CURRENTLY_ON(BrowserThread::IO); - CHECK(audio_entries_.empty()); + DCHECK(delegates_.empty()); // If we had any streams, report UMA stats for the maximum number of // simultaneous streams for this render process and for the whole browser @@ -226,10 +124,10 @@ void AudioRendererHost::OnChannelClosing() { DCHECK_CURRENTLY_ON(BrowserThread::IO); // Since the IPC sender is gone, close all requested audio streams. - while (!audio_entries_.empty()) { - // Note: OnCloseStream() removes the entries from audio_entries_. - OnCloseStream(audio_entries_.begin()->first); - } + // The audio streams tracker isn't automatically decremented since the + // removal isn't done through OnCloseStream. + g_audio_streams_tracker.Get().DecreaseStreamCount(delegates_.size()); + delegates_.clear(); // Remove any authorizations for streams that were not yet created authorizations_.clear(); @@ -239,53 +137,23 @@ BrowserThread::DeleteOnIOThread::Destruct(this); } -void AudioRendererHost::AudioEntry::OnCreated() { - BrowserThread::PostTask( - BrowserThread::IO, - FROM_HERE, - base::Bind(&AudioRendererHost::DoCompleteCreation, host_, stream_id_)); -} - -void AudioRendererHost::AudioEntry::OnPlaying() { - BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, - base::Bind(&AudioRendererHost::StreamStateChanged, - host_, stream_id_, true)); -} - -void AudioRendererHost::AudioEntry::OnPaused() { - BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, - base::Bind(&AudioRendererHost::StreamStateChanged, - host_, stream_id_, false)); -} - -void AudioRendererHost::AudioEntry::OnError() { - BrowserThread::PostTask( - BrowserThread::IO, - FROM_HERE, - base::Bind(&AudioRendererHost::ReportErrorAndClose, host_, stream_id_)); -} - -void AudioRendererHost::DoCompleteCreation(int stream_id) { +void AudioRendererHost::OnStreamCreated( + int stream_id, + base::SharedMemory* shared_memory, + base::CancelableSyncSocket* foreign_socket) { DCHECK_CURRENTLY_ON(BrowserThread::IO); if (!PeerHandle()) { DLOG(WARNING) << "Renderer process handle is invalid."; - ReportErrorAndClose(stream_id); + OnStreamError(stream_id); return; } - AudioEntry* const entry = LookupById(stream_id); - if (!entry) { - ReportErrorAndClose(stream_id); + if (!LookupById(stream_id)) { + OnStreamError(stream_id); return; } - // Now construction is done and we are ready to send the shared memory and the - // sync socket to the renderer. - base::SharedMemory* shared_memory = entry->reader()->shared_memory(); - base::CancelableSyncSocket* foreign_socket = - entry->reader()->foreign_socket(); - base::SharedMemoryHandle foreign_memory_handle; base::SyncSocket::TransitDescriptor socket_descriptor; size_t shared_memory_size = shared_memory->requested_size(); @@ -294,7 +162,7 @@ foreign_socket->PrepareTransitDescriptor(PeerHandle(), &socket_descriptor))) { // Something went wrong in preparing the IPC handles. - ReportErrorAndClose(entry->stream_id()); + OnStreamError(stream_id); return; } @@ -303,35 +171,22 @@ base::checked_cast<uint32_t>(shared_memory_size))); } +void AudioRendererHost::OnStreamError(int stream_id) { + DCHECK_CURRENTLY_ON(BrowserThread::IO); + + SendErrorMessage(stream_id); + OnCloseStream(stream_id); +} + void AudioRendererHost::DidValidateRenderFrame(int stream_id, bool is_valid) { DCHECK_CURRENTLY_ON(BrowserThread::IO); - if (!is_valid) { - DLOG(WARNING) << "Render frame for stream (id=" << stream_id - << ") no longer exists."; - ReportErrorAndClose(stream_id); - } -} - -void AudioRendererHost::StreamStateChanged(int stream_id, bool is_playing) { - DCHECK_CURRENTLY_ON(BrowserThread::IO); - - AudioEntry* const entry = LookupById(stream_id); - if (!entry) + if (is_valid) return; - if (is_playing) { - AudioStreamMonitor::StartMonitoringStream( - render_process_id_, - entry->render_frame_id(), - entry->stream_id(), - base::Bind(&media::AudioOutputController::ReadCurrentPowerAndClip, - entry->controller())); - } else { - AudioStreamMonitor::StopMonitoringStream( - render_process_id_, entry->render_frame_id(), entry->stream_id()); - } - UpdateNumPlayingStreams(entry, is_playing); + DLOG(WARNING) << "Render frame for stream (id=" << stream_id + << ") no longer exists."; + OnStreamError(stream_id); } RenderProcessHost::AudioOutputControllerList @@ -339,11 +194,8 @@ DCHECK_CURRENTLY_ON(BrowserThread::IO); RenderProcessHost::AudioOutputControllerList controllers; - for (AudioEntryMap::const_iterator it = audio_entries_.begin(); - it != audio_entries_.end(); - ++it) { - controllers.push_back(it->second->controller()); - } + for (const auto& delegate : delegates_) + controllers.push_back(delegate->controller()); return controllers; } @@ -379,6 +231,7 @@ << ", render_frame_id=" << render_frame_id << ", session_id=" << session_id << ", device_id=" << device_id << ", security_origin=" << security_origin << ")"; + if (LookupById(stream_id) || IsAuthorizationStarted(stream_id)) return; @@ -453,8 +306,9 @@ } // Fail early if either of two sanity-checks fail: - // 1. There should not yet exist an AudioEntry for the given |stream_id| - // since the renderer may not create two streams with the same ID. + // 1. There should not yet exist an AudioOutputDelegate for the given + // |stream_id| since the renderer may not create two streams with the + // same ID. // 2. An out-of-range render frame ID was provided. Renderers must *always* // specify a valid render frame ID for each audio output they create, as // several browser-level features depend on this (e.g., OOM manager, UI @@ -481,64 +335,54 @@ base::Bind(&AudioRendererHost::DidValidateRenderFrame, this, stream_id))); - std::unique_ptr<AudioEntry> entry = AudioEntry::Create( - this, stream_id, render_frame_id, params, device_unique_id); - if (!entry) { - SendErrorMessage(stream_id); - return; - } - MediaObserver* const media_observer = GetContentClient()->browser()->GetMediaObserver(); - if (media_observer) - media_observer->OnCreatingAudioStream(render_process_id_, render_frame_id); - if (mirroring_manager_) { - mirroring_manager_->AddDiverter( - render_process_id_, entry->render_frame_id(), entry->controller()); - } - audio_entries_.insert(std::make_pair(stream_id, entry.release())); + MediaInternals* const media_internals = MediaInternals::GetInstance(); + std::unique_ptr<media::AudioLog> audio_log = media_internals->CreateAudioLog( + media::AudioLogFactory::AUDIO_OUTPUT_CONTROLLER); + media_internals->SetWebContentsTitleForAudioLogEntry( + stream_id, render_process_id_, render_frame_id, audio_log.get()); + delegates_.push_back(AudioOutputDelegate::Create( + this, audio_manager_, std::move(audio_log), mirroring_manager_, + media_observer, stream_id, render_frame_id, render_process_id_, params, + device_unique_id)); + g_audio_streams_tracker.Get().IncreaseStreamCount(); - audio_log_->OnCreated(stream_id, params, device_unique_id); - MediaInternals::GetInstance()->SetWebContentsTitleForAudioLogEntry( - stream_id, render_process_id_, render_frame_id, audio_log_.get()); - - if (audio_entries_.size() > max_simultaneous_streams_) - max_simultaneous_streams_ = audio_entries_.size(); + if (delegates_.size() > max_simultaneous_streams_) + max_simultaneous_streams_ = delegates_.size(); } void AudioRendererHost::OnPlayStream(int stream_id) { DCHECK_CURRENTLY_ON(BrowserThread::IO); - AudioEntry* entry = LookupById(stream_id); - if (!entry) { + AudioOutputDelegate* delegate = LookupById(stream_id); + if (!delegate) { SendErrorMessage(stream_id); return; } - entry->controller()->Play(); - audio_log_->OnStarted(stream_id); + delegate->OnPlayStream(); } void AudioRendererHost::OnPauseStream(int stream_id) { DCHECK_CURRENTLY_ON(BrowserThread::IO); - AudioEntry* entry = LookupById(stream_id); - if (!entry) { + AudioOutputDelegate* delegate = LookupById(stream_id); + if (!delegate) { SendErrorMessage(stream_id); return; } - entry->controller()->Pause(); - audio_log_->OnStopped(stream_id); + delegate->OnPauseStream(); } void AudioRendererHost::OnSetVolume(int stream_id, double volume) { DCHECK_CURRENTLY_ON(BrowserThread::IO); - AudioEntry* entry = LookupById(stream_id); - if (!entry) { + AudioOutputDelegate* delegate = LookupById(stream_id); + if (!delegate) { SendErrorMessage(stream_id); return; } @@ -546,8 +390,7 @@ // Make sure the volume is valid. if (volume < 0 || volume > 1.0) return; - entry->controller()->SetVolume(volume); - audio_log_->OnSetVolume(stream_id, volume); + delegate->OnSetVolume(volume); } void AudioRendererHost::SendErrorMessage(int stream_id) { @@ -558,71 +401,44 @@ DCHECK_CURRENTLY_ON(BrowserThread::IO); authorizations_.erase(stream_id); + auto i = LookupIteratorById(stream_id); + // Prevent oustanding callbacks from attempting to close/delete the same - // AudioEntry twice. - AudioEntryMap::iterator i = audio_entries_.find(stream_id); - if (i == audio_entries_.end()) + // AudioOutputDelegate twice. + if (i == delegates_.end()) return; - std::unique_ptr<AudioEntry> entry(i->second); - audio_entries_.erase(i); + + std::swap(*i, delegates_.back()); + delegates_.pop_back(); + g_audio_streams_tracker.Get().DecreaseStreamCount(); - - media::AudioOutputController* const controller = entry->controller(); - controller->Close( - base::Bind(&AudioRendererHost::DeleteEntry, this, base::Passed(&entry))); - audio_log_->OnClosed(stream_id); } -void AudioRendererHost::DeleteEntry(std::unique_ptr<AudioEntry> entry) { +AudioRendererHost::AudioOutputDelegateVector::iterator +AudioRendererHost::LookupIteratorById(int stream_id) { DCHECK_CURRENTLY_ON(BrowserThread::IO); - // De-register the controller from the AudioMirroringManager now that the - // controller has closed the AudioOutputStream and shut itself down. This - // ensures that calling RemoveDiverter() here won't trigger the controller to - // re-start the default AudioOutputStream and cause a brief audio blip to come - // out the user's speakers. http://crbug.com/474432 - if (mirroring_manager_) - mirroring_manager_->RemoveDiverter(entry->controller()); - - AudioStreamMonitor::StopMonitoringStream( - render_process_id_, entry->render_frame_id(), entry->stream_id()); - UpdateNumPlayingStreams(entry.get(), false); + return std::find_if(delegates_.begin(), delegates_.end(), + [stream_id](const AudioOutputDelegate::UniquePtr& d) { + return d->stream_id() == stream_id; + }); } -void AudioRendererHost::ReportErrorAndClose(int stream_id) { +AudioOutputDelegate* AudioRendererHost::LookupById(int stream_id) { DCHECK_CURRENTLY_ON(BrowserThread::IO); - // Make sure this isn't a stray callback executing after the stream has been - // closed, so error notifications aren't sent after clients believe the stream - // is closed. - if (!LookupById(stream_id)) - return; - - SendErrorMessage(stream_id); - - audio_log_->OnError(stream_id); - OnCloseStream(stream_id); + auto i = LookupIteratorById(stream_id); + return i != delegates_.end() ? i->get() : nullptr; } -AudioRendererHost::AudioEntry* AudioRendererHost::LookupById(int stream_id) { +void AudioRendererHost::OnStreamStateChanged(bool is_playing) { DCHECK_CURRENTLY_ON(BrowserThread::IO); - - AudioEntryMap::const_iterator i = audio_entries_.find(stream_id); - return i != audio_entries_.end() ? i->second : NULL; -} - -void AudioRendererHost::UpdateNumPlayingStreams(AudioEntry* entry, - bool is_playing) { - DCHECK_CURRENTLY_ON(BrowserThread::IO); - if (entry->playing() == is_playing) - return; - if (is_playing) { - entry->set_playing(true); base::AtomicRefCountInc(&num_playing_streams_); // Inform the RenderProcessHost when audio starts playing for the first - // time. + // time. The nonatomic increment-and-read is ok since this is the only + // thread that |num_plaing_streams_| may be updated on. if (base::AtomicRefCountIsOne(&num_playing_streams_)) { BrowserThread::PostTask( BrowserThread::UI, FROM_HERE, @@ -630,7 +446,6 @@ render_process_id_)); } } else { - entry->set_playing(false); // Inform the RenderProcessHost when there is no more audio playing. if (!base::AtomicRefCountDec(&num_playing_streams_)) { BrowserThread::PostTask(
diff --git a/content/browser/renderer_host/media/audio_renderer_host.h b/content/browser/renderer_host/media/audio_renderer_host.h index 2fa7d35..ad03140 100644 --- a/content/browser/renderer_host/media/audio_renderer_host.h +++ b/content/browser/renderer_host/media/audio_renderer_host.h
@@ -40,33 +40,22 @@ #ifndef CONTENT_BROWSER_RENDERER_HOST_MEDIA_AUDIO_RENDERER_HOST_H_ #define CONTENT_BROWSER_RENDERER_HOST_MEDIA_AUDIO_RENDERER_HOST_H_ -#include <stddef.h> - #include <map> #include <memory> #include <string> #include <utility> +#include <vector> -#include "base/atomic_ref_count.h" -#include "base/gtest_prod_util.h" -#include "base/logging.h" -#include "base/macros.h" -#include "base/memory/ref_counted.h" -#include "base/process/process.h" -#include "base/sequenced_task_runner_helpers.h" #include "content/browser/renderer_host/media/audio_output_authorization_handler.h" -#include "content/browser/renderer_host/media/media_devices_manager.h" +#include "content/browser/renderer_host/media/audio_output_delegate.h" #include "content/common/content_export.h" #include "content/public/browser/browser_message_filter.h" -#include "content/public/browser/browser_thread.h" #include "content/public/browser/render_process_host.h" -#include "content/public/browser/resource_context.h" -#include "media/audio/audio_io.h" -#include "media/audio/audio_logging.h" -#include "media/audio/audio_output_controller.h" -#include "media/audio/simple_sources.h" -#include "media/base/output_device_info.h" -#include "url/origin.h" + +namespace base { +class SharedMemory; +class CancelableSyncSocket; +} namespace media { class AudioManager; @@ -76,16 +65,16 @@ namespace content { class AudioMirroringManager; -class MediaInternals; class MediaStreamManager; -class CONTENT_EXPORT AudioRendererHost : public BrowserMessageFilter { +class CONTENT_EXPORT AudioRendererHost + : public BrowserMessageFilter, + public AudioOutputDelegate::EventHandler { public: // Called from UI thread from the owner of this object. AudioRendererHost(int render_process_id, media::AudioManager* audio_manager, AudioMirroringManager* mirroring_manager, - MediaInternals* media_internals, MediaStreamManager* media_stream_manager, const std::string& salt); @@ -99,6 +88,13 @@ void OnDestruct() const override; bool OnMessageReceived(const IPC::Message& message) override; + // AudioOutputDelegate::EventHandler implementation + void OnStreamCreated(int stream_id, + base::SharedMemory* shared_memory, + base::CancelableSyncSocket* foreign_socket) override; + void OnStreamError(int stream_id) override; + void OnStreamStateChanged(bool is_playing) override; + // Returns true if any streams managed by this host are actively playing. Can // be called from any thread. bool HasActiveAudio(); @@ -114,13 +110,12 @@ FRIEND_TEST_ALL_PREFIXES(AudioRendererHostTest, CreateMockStream); FRIEND_TEST_ALL_PREFIXES(AudioRendererHostTest, MockStreamDataConversation); - class AudioEntry; - typedef std::map<int, AudioEntry*> AudioEntryMap; - // Internal callback type for access requests to output devices. // |have_access| is true only if there is permission to access the device. typedef base::Callback<void(bool have_access)> OutputDeviceAccessCB; + using AudioOutputDelegateVector = std::vector<AudioOutputDelegate::UniquePtr>; + // The type of a function that is run on the UI thread to check whether the // routing IDs reference a valid RenderFrameHost. The function then runs // |callback| on the IO thread with true/false if valid/invalid. @@ -178,13 +173,8 @@ // Helper methods. - // Complete the process of creating an audio stream. This will set up the - // shared memory or shared socket in low latency mode and send the - // NotifyStreamCreated message to the peer. - void DoCompleteCreation(int stream_id); - // Called after the |render_frame_id| provided to OnCreateStream() was - // validated. When |is_valid| is false, this calls ReportErrorAndClose(). + // validated. When |is_valid| is false, this calls OnStreamError(). void DidValidateRenderFrame(int stream_id, bool is_valid); // Updates status of stream for AudioStreamMonitor and updates @@ -196,20 +186,11 @@ // Send an error message to the renderer. void SendErrorMessage(int stream_id); - // Delete an audio entry, notifying observers first. This is called by - // AudioOutputController after it has closed. - void DeleteEntry(std::unique_ptr<AudioEntry> entry); - - // Send an error message to the renderer, then close the stream. - void ReportErrorAndClose(int stream_id); - - // A helper method to look up a AudioEntry identified by |stream_id|. - // Returns NULL if not found. - AudioEntry* LookupById(int stream_id); - - // A helper method to update the number of playing streams and alert the - // ResourceScheduler when the renderer starts or stops playing an audiostream. - void UpdateNumPlayingStreams(AudioEntry* entry, bool is_playing); + // Helper methods to look up a AudioOutputDelegate identified by |stream_id|. + // Returns delegates_.end() if not found. + AudioOutputDelegateVector::iterator LookupIteratorById(int stream_id); + // Returns nullptr if not found. + AudioOutputDelegate* LookupById(int stream_id); // Helper method to check if the authorization procedure for stream // |stream_id| has started. @@ -227,10 +208,12 @@ media::AudioManager* const audio_manager_; AudioMirroringManager* const mirroring_manager_; - std::unique_ptr<media::AudioLog> audio_log_; - // A map of stream IDs to audio sources. - AudioEntryMap audio_entries_; + // Used to access to AudioInputDeviceManager. + MediaStreamManager* media_stream_manager_; + + // A list of the current open streams. + AudioOutputDelegateVector delegates_; // The number of streams in the playing state. Atomic read safe from any // thread, but should only be updated from the IO thread.
diff --git a/content/browser/renderer_host/media/audio_renderer_host_unittest.cc b/content/browser/renderer_host/media/audio_renderer_host_unittest.cc index e7542b9..04b7209b 100644 --- a/content/browser/renderer_host/media/audio_renderer_host_unittest.cc +++ b/content/browser/renderer_host/media/audio_renderer_host_unittest.cc
@@ -127,13 +127,11 @@ int render_process_id, media::AudioManager* audio_manager, AudioMirroringManager* mirroring_manager, - MediaInternals* media_internals, MediaStreamManager* media_stream_manager, const std::string& salt) : AudioRendererHost(render_process_id, audio_manager, mirroring_manager, - media_internals, media_stream_manager, salt), shared_memory_length_(0), @@ -147,8 +145,8 @@ media::OutputDeviceStatus device_status, const media::AudioParameters& output_params, const std::string& matched_device_id)); - MOCK_METHOD2(OnStreamCreated, void(int stream_id, int length)); - MOCK_METHOD1(OnStreamError, void(int stream_id)); + MOCK_METHOD2(WasNotifiedOfCreation, void(int stream_id, int length)); + MOCK_METHOD1(WasNotifiedOfError, void(int stream_id)); void ShutdownForBadMessage() override { bad_msg_count++; } @@ -157,7 +155,7 @@ private: virtual ~MockAudioRendererHost() { // Make sure all audio streams have been deleted. - EXPECT_TRUE(audio_entries_.empty()); + EXPECT_TRUE(delegates_.empty()); } // This method is used to dispatch IPC messages to the renderer. We intercept @@ -215,10 +213,10 @@ sync_socket_.reset(new base::SyncSocket(sync_socket_handle)); // And then delegate the call to the mock method. - OnStreamCreated(stream_id, length); + WasNotifiedOfCreation(stream_id, length); } - void OnNotifyStreamError(int stream_id) { OnStreamError(stream_id); } + void OnNotifyStreamError(int stream_id) { WasNotifiedOfError(stream_id); } std::unique_ptr<base::SharedMemory> shared_memory_; std::unique_ptr<base::SyncSocket> sync_socket_; @@ -241,8 +239,7 @@ media_stream_manager_.reset(new MediaStreamManager(audio_manager_.get())); host_ = new MockAudioRendererHost( &auth_run_loop_, render_process_host_.GetID(), audio_manager_.get(), - &mirroring_manager_, MediaInternals::GetInstance(), - media_stream_manager_.get(), kSalt); + &mirroring_manager_, media_stream_manager_.get(), kSalt); // Simulate IPC channel connected. host_->set_peer_process_for_testing(base::Process::Current()); @@ -341,7 +338,7 @@ OnDeviceAuthorized(kStreamId, expected_device_status, _, _)); if (expected_device_status == media::OUTPUT_DEVICE_STATUS_OK) { - EXPECT_CALL(*host_.get(), OnStreamCreated(kStreamId, _)); + EXPECT_CALL(*host_.get(), WasNotifiedOfCreation(kStreamId, _)); EXPECT_CALL(mirroring_manager_, AddDiverter(render_process_host_.GetID(), kRenderFrameId, NotNull())) .RetiresOnSaturation(); @@ -387,11 +384,11 @@ void CreateWithInvalidRenderFrameId() { // When creating a stream with an invalid render frame ID, the host will // reply with a stream error message. - EXPECT_CALL(*host_, OnStreamError(kStreamId)); + EXPECT_CALL(*host_, WasNotifiedOfError(kStreamId)); // However, validation does not block stream creation, so these method calls // might be made: - EXPECT_CALL(*host_, OnStreamCreated(kStreamId, _)).Times(AtLeast(0)); + EXPECT_CALL(*host_, WasNotifiedOfCreation(kStreamId, _)).Times(AtLeast(0)); EXPECT_CALL(mirroring_manager_, AddDiverter(_, _, _)).Times(AtLeast(0)); EXPECT_CALL(mirroring_manager_, RemoveDiverter(_)).Times(AtLeast(0)); @@ -430,7 +427,7 @@ OnDeviceAuthorized(kStreamId, media::OUTPUT_DEVICE_STATUS_OK, _, hashed_output_id)) .Times(1); - EXPECT_CALL(*host_.get(), OnStreamCreated(kStreamId, _)); + EXPECT_CALL(*host_.get(), WasNotifiedOfCreation(kStreamId, _)); EXPECT_CALL(mirroring_manager_, AddDiverter(render_process_host_.GetID(), kRenderFrameId, NotNull())) .RetiresOnSaturation(); @@ -471,18 +468,18 @@ } void SimulateError() { - EXPECT_EQ(1u, host_->audio_entries_.size()) + EXPECT_EQ(1u, host_->delegates_.size()) << "Calls Create() before calling this method"; // Expect an error signal sent through IPC. - EXPECT_CALL(*host_.get(), OnStreamError(kStreamId)); + EXPECT_CALL(*host_.get(), WasNotifiedOfError(kStreamId)); // Simulate an error sent from the audio device. - host_->ReportErrorAndClose(kStreamId); + host_->OnStreamError(kStreamId); SyncWithAudioThread(); // Expect the audio stream record is removed. - EXPECT_EQ(0u, host_->audio_entries_.size()); + EXPECT_EQ(0u, host_->delegates_.size()); } // SyncWithAudioThread() waits until all pending tasks on the audio thread
diff --git a/content/browser/renderer_host/render_process_host_impl.cc b/content/browser/renderer_host/render_process_host_impl.cc index 42c6716..72f3b1c 100644 --- a/content/browser/renderer_host/render_process_host_impl.cc +++ b/content/browser/renderer_host/render_process_host_impl.cc
@@ -1070,7 +1070,7 @@ AddFilter(audio_input_renderer_host_.get()); audio_renderer_host_ = new AudioRendererHost( GetID(), audio_manager, AudioMirroringManager::GetInstance(), - media_internals, media_stream_manager, + media_stream_manager, browser_context->GetResourceContext()->GetMediaDeviceIDSalt()); AddFilter(audio_renderer_host_.get()); AddFilter(
diff --git a/content/public/common/content_features.cc b/content/public/common/content_features.cc index 1bea0d7..ff879647 100644 --- a/content/public/common/content_features.cc +++ b/content/public/common/content_features.cc
@@ -242,6 +242,12 @@ const base::Feature kSeccompSandboxAndroid{"SeccompSandboxAndroid", base::FEATURE_DISABLED_BY_DEFAULT}; +// Service worker based payment apps as defined by w3c here: +// https://w3c.github.io/webpayments-payment-apps-api/ +const base::Feature kServiceWorkerPaymentApps{ + "ServiceWorkerPaymentApps", + base::FEATURE_DISABLED_BY_DEFAULT}; + // The JavaScript API for payments on the web. const base::Feature kWebPayments{"WebPayments", base::FEATURE_ENABLED_BY_DEFAULT};
diff --git a/content/public/common/content_features.h b/content/public/common/content_features.h index 4230595..7d33386d 100644 --- a/content/public/common/content_features.h +++ b/content/public/common/content_features.h
@@ -67,6 +67,7 @@ #if defined(OS_ANDROID) CONTENT_EXPORT extern const base::Feature kImeThread; CONTENT_EXPORT extern const base::Feature kSeccompSandboxAndroid; +CONTENT_EXPORT extern const base::Feature kServiceWorkerPaymentApps; CONTENT_EXPORT extern const base::Feature kWebPayments; #endif // defined(OS_ANDROID)
diff --git a/content/test/BUILD.gn b/content/test/BUILD.gn index c191363..0550e19 100644 --- a/content/test/BUILD.gn +++ b/content/test/BUILD.gn
@@ -1186,6 +1186,7 @@ "../browser/renderer_host/media/audio_input_device_manager_unittest.cc", "../browser/renderer_host/media/audio_input_sync_writer_unittest.cc", "../browser/renderer_host/media/audio_output_authorization_handler_unittest.cc", + "../browser/renderer_host/media/audio_output_delegate_unittest.cc", "../browser/renderer_host/media/audio_renderer_host_unittest.cc", "../browser/renderer_host/media/media_devices_dispatcher_host_unittest.cc", "../browser/renderer_host/media/media_devices_manager_unittest.cc",
diff --git a/ios/chrome/app/strings/ios_strings.grd b/ios/chrome/app/strings/ios_strings.grd index 9e80d65..4dd9065 100644 --- a/ios/chrome/app/strings/ios_strings.grd +++ b/ios/chrome/app/strings/ios_strings.grd
@@ -709,7 +709,7 @@ <message name="IDS_IOS_ICON_SEARCH" desc="Accessibility label for the search icon [iOS only]."> Search </message> - <message name="IDS_IOS_JAVA_SCRIPT_DIALOG_BLOCKING_BUTTON_TEXT" desc="The text used for the button added to JavaScript dialogs to prevent a page from spamming the user with endless alerts. [Length:20em]"> + <message name="IDS_IOS_JAVA_SCRIPT_DIALOG_BLOCKING_BUTTON_TEXT" desc="The text used for the button added to JavaScript dialogs to prevent a page from spamming the user with endless alerts. [Length:25em]"> Suppress Dialogs </message> <message name="IDS_IOS_KEYBOARD_BOOKMARK_THIS_PAGE" desc="In Title Case: Title of the keyboard shortcut that bookmarks the current page. [Length: 20 em] [iOS only]">
diff --git a/ios/chrome/browser/browser_state/BUILD.gn b/ios/chrome/browser/browser_state/BUILD.gn index ac928b8..50c1e4f 100644 --- a/ios/chrome/browser/browser_state/BUILD.gn +++ b/ios/chrome/browser/browser_state/BUILD.gn
@@ -153,4 +153,6 @@ "//ios/web", "//net:test_support", ] + + configs += [ "//build/config/compiler:enable_arc" ] }
diff --git a/ios/chrome/browser/browser_state/test_chrome_browser_state.mm b/ios/chrome/browser/browser_state/test_chrome_browser_state.mm index 9f79bc05..c38e532e 100644 --- a/ios/chrome/browser/browser_state/test_chrome_browser_state.mm +++ b/ios/chrome/browser/browser_state/test_chrome_browser_state.mm
@@ -48,6 +48,10 @@ #include "ios/web/public/web_thread.h" #include "net/url_request/url_request_test_util.h" +#if !defined(__has_feature) || !__has_feature(objc_arc) +#error "This file requires ARC support." +#endif + namespace { std::unique_ptr<KeyedService> BuildHistoryService(web::BrowserState* context) { ios::ChromeBrowserState* browser_state =
diff --git a/ios/chrome/browser/browser_state/test_chrome_browser_state_isolated_context.mm b/ios/chrome/browser/browser_state/test_chrome_browser_state_isolated_context.mm index 9bf55a26..04c3d10f 100644 --- a/ios/chrome/browser/browser_state/test_chrome_browser_state_isolated_context.mm +++ b/ios/chrome/browser/browser_state/test_chrome_browser_state_isolated_context.mm
@@ -9,6 +9,10 @@ #include "ios/web/public/web_thread.h" #include "net/url_request/url_request_test_util.h" +#if !defined(__has_feature) || !__has_feature(objc_arc) +#error "This file requires ARC support." +#endif + TestChromeBrowserStateWithIsolatedContext:: TestChromeBrowserStateWithIsolatedContext() : TestChromeBrowserState(
diff --git a/ios/chrome/browser/first_run/BUILD.gn b/ios/chrome/browser/first_run/BUILD.gn index b189a431..25f50590 100644 --- a/ios/chrome/browser/first_run/BUILD.gn +++ b/ios/chrome/browser/first_run/BUILD.gn
@@ -3,6 +3,7 @@ # found in the LICENSE file. source_set("first_run") { + configs += [ "//build/config/compiler:enable_arc" ] sources = [ "first_run.h", "first_run.mm",
diff --git a/ios/chrome/browser/first_run/first_run.mm b/ios/chrome/browser/first_run/first_run.mm index 553cf20..4f88454 100644 --- a/ios/chrome/browser/first_run/first_run.mm +++ b/ios/chrome/browser/first_run/first_run.mm
@@ -10,6 +10,10 @@ #include "components/pref_registry/pref_registry_syncable.h" #include "ios/chrome/browser/chrome_paths.h" +#if !defined(__has_feature) || !__has_feature(objc_arc) +#error "This file requires ARC support." +#endif + namespace { // The absence of kSentinelFile file will tell us it is a first run.
diff --git a/ios/chrome/browser/first_run/first_run_configuration.mm b/ios/chrome/browser/first_run/first_run_configuration.mm index 7da06913..830df36 100644 --- a/ios/chrome/browser/first_run/first_run_configuration.mm +++ b/ios/chrome/browser/first_run/first_run_configuration.mm
@@ -4,6 +4,10 @@ #import "ios/chrome/browser/first_run/first_run_configuration.h" +#if !defined(__has_feature) || !__has_feature(objc_arc) +#error "This file requires ARC support." +#endif + @implementation FirstRunConfiguration @synthesize signInAttempted = _signInAttempted;
diff --git a/ios/chrome/browser/geolocation/BUILD.gn b/ios/chrome/browser/geolocation/BUILD.gn index 7273ecd..d386fa59 100644 --- a/ios/chrome/browser/geolocation/BUILD.gn +++ b/ios/chrome/browser/geolocation/BUILD.gn
@@ -3,6 +3,7 @@ # found in the LICENSE file. source_set("geolocation") { + configs += [ "//build/config/compiler:enable_arc" ] sources = [ "CLLocation+OmniboxGeolocation.h", "CLLocation+OmniboxGeolocation.mm", @@ -33,6 +34,7 @@ } source_set("test_support") { + configs += [ "//build/config/compiler:enable_arc" ] testonly = true sources = [ "test_location_manager.h",
diff --git a/ios/chrome/browser/geolocation/CLLocation+OmniboxGeolocation.mm b/ios/chrome/browser/geolocation/CLLocation+OmniboxGeolocation.mm index 1449373..8a25eb4 100644 --- a/ios/chrome/browser/geolocation/CLLocation+OmniboxGeolocation.mm +++ b/ios/chrome/browser/geolocation/CLLocation+OmniboxGeolocation.mm
@@ -6,7 +6,9 @@ #import <objc/runtime.h> -#include "base/mac/scoped_nsobject.h" +#if !defined(__has_feature) || !__has_feature(objc_arc) +#error "This file requires ARC support." +#endif namespace { @@ -32,9 +34,8 @@ } - (void)cr_setAcquisitionInterval:(NSTimeInterval)interval { - base::scoped_nsobject<NSNumber> boxedInterval( - [[NSNumber alloc] initWithDouble:interval]); - objc_setAssociatedObject(self, &g_acquisitionIntervalKey, boxedInterval.get(), + NSNumber* boxedInterval = [[NSNumber alloc] initWithDouble:interval]; + objc_setAssociatedObject(self, &g_acquisitionIntervalKey, boxedInterval, OBJC_ASSOCIATION_RETAIN); }
diff --git a/ios/chrome/browser/geolocation/CLLocation+XGeoHeader.mm b/ios/chrome/browser/geolocation/CLLocation+XGeoHeader.mm index f846c47..e7a4299 100644 --- a/ios/chrome/browser/geolocation/CLLocation+XGeoHeader.mm +++ b/ios/chrome/browser/geolocation/CLLocation+XGeoHeader.mm
@@ -8,6 +8,10 @@ #import "third_party/google_toolbox_for_mac/src/Foundation/GTMStringEncoding.h" +#if !defined(__has_feature) || !__has_feature(objc_arc) +#error "This file requires ARC support." +#endif + NSString* const kGMOLocationDescriptorFormat = @"role: CURRENT_LOCATION\n" @"producer: DEVICE_LOCATION\n" @@ -28,9 +32,8 @@ error:nullptr]; if (base64) { return base64; - } else { - return @""; } + return @""; } // Returns the timestamp of this location in microseconds since the UNIX epoch.
diff --git a/ios/chrome/browser/geolocation/location_manager.h b/ios/chrome/browser/geolocation/location_manager.h index 1af7b5c..a8a12ad 100644 --- a/ios/chrome/browser/geolocation/location_manager.h +++ b/ios/chrome/browser/geolocation/location_manager.h
@@ -29,10 +29,10 @@ @property(nonatomic, readonly) CLAuthorizationStatus authorizationStatus; // Returns the most recently fetched location. -@property(nonatomic, readonly) CLLocation* currentLocation; +@property(strong, nonatomic, readonly) CLLocation* currentLocation; // The delegate object for this instance of LocationManager. -@property(nonatomic, assign) id<LocationManagerDelegate> delegate; +@property(weak, nonatomic) id<LocationManagerDelegate> delegate; // Boolean value indicating whether location services are enabled on the // device. This proxies |[CLLocationManager locationServicesEnabled]|, so that
diff --git a/ios/chrome/browser/geolocation/location_manager.mm b/ios/chrome/browser/geolocation/location_manager.mm index cab3ea7..e9daf6e 100644 --- a/ios/chrome/browser/geolocation/location_manager.mm +++ b/ios/chrome/browser/geolocation/location_manager.mm
@@ -4,13 +4,15 @@ #import "ios/chrome/browser/geolocation/location_manager.h" -#import "base/ios/weak_nsobject.h" -#include "base/mac/scoped_nsobject.h" #import "ios/chrome/browser/geolocation/CLLocation+OmniboxGeolocation.h" #import "ios/chrome/browser/geolocation/location_manager+Testing.h" #import "ios/public/provider/chrome/browser/chrome_browser_provider.h" #import "ios/public/provider/chrome/browser/geolocation_updater_provider.h" +#if !defined(__has_feature) || !__has_feature(objc_arc) +#error "This file requires ARC support." +#endif + namespace { const CLLocationDistance kLocationDesiredAccuracy = @@ -23,10 +25,8 @@ } // namespace @interface LocationManager () { - base::scoped_nsprotocol<id<GeolocationUpdater>> _locationUpdater; - base::scoped_nsobject<CLLocation> _currentLocation; - base::WeakNSProtocol<id<LocationManagerDelegate>> _delegate; - base::scoped_nsobject<NSDate> _startTime; + id<GeolocationUpdater> _locationUpdater; + NSDate* _startTime; } // Handles GeolocationUpdater notification for an updated device location. @@ -39,6 +39,8 @@ @end @implementation LocationManager +@synthesize delegate = _delegate; +@synthesize currentLocation = _currentLocation; - (id)init { self = [super init]; @@ -48,7 +50,7 @@ // |provider| may be null in tests. if (provider) { - _locationUpdater.reset(provider->CreateGeolocationUpdater(false)); + _locationUpdater = provider->CreateGeolocationUpdater(false); [_locationUpdater setDesiredAccuracy:kLocationDesiredAccuracy distanceFilter:kLocationDesiredAccuracy / 2]; [_locationUpdater setStopUpdateDelay:kLocationStopUpdateDelay]; @@ -76,7 +78,6 @@ - (void)dealloc { [[NSNotificationCenter defaultCenter] removeObserver:self]; - [super dealloc]; } - (CLAuthorizationStatus)authorizationStatus { @@ -85,18 +86,10 @@ - (CLLocation*)currentLocation { if (!_currentLocation) - _currentLocation.reset([[_locationUpdater currentLocation] retain]); + _currentLocation = [_locationUpdater currentLocation]; return _currentLocation; } -- (id<LocationManagerDelegate>)delegate { - return _delegate; -} - -- (void)setDelegate:(id<LocationManagerDelegate>)delegate { - _delegate.reset(delegate); -} - - (BOOL)locationServicesEnabled { return [CLLocationManager locationServicesEnabled]; } @@ -105,7 +98,7 @@ CLLocation* currentLocation = self.currentLocation; if (!currentLocation || [currentLocation cr_shouldRefresh]) { if (![_locationUpdater isEnabled]) - _startTime.reset([[NSDate alloc] init]); + _startTime = [[NSDate alloc] init]; [_locationUpdater requestWhenInUseAuthorization]; [_locationUpdater setEnabled:YES]; @@ -124,7 +117,7 @@ ->GetUpdateNewLocationKey(); CLLocation* location = [[notification userInfo] objectForKey:newLocationKey]; if (location) { - _currentLocation.reset([location retain]); + _currentLocation = location; if (_startTime) { NSTimeInterval interval = -[_startTime timeIntervalSinceNow]; @@ -144,7 +137,7 @@ #pragma mark - LocationManager+Testing - (void)setGeolocationUpdater:(id<GeolocationUpdater>)geolocationUpdater { - _locationUpdater.reset([geolocationUpdater retain]); + _locationUpdater = geolocationUpdater; } @end
diff --git a/ios/chrome/browser/geolocation/omnibox_geolocation_authorization_alert.h b/ios/chrome/browser/geolocation/omnibox_geolocation_authorization_alert.h index f753924..ff2db73 100644 --- a/ios/chrome/browser/geolocation/omnibox_geolocation_authorization_alert.h +++ b/ios/chrome/browser/geolocation/omnibox_geolocation_authorization_alert.h
@@ -30,8 +30,8 @@ @interface OmniboxGeolocationAuthorizationAlert : NSObject // The delegate for this OmniboxGeolocationAuthorizationAlert. -@property(nonatomic, assign) - id<OmniboxGeolocationAuthorizationAlertDelegate> delegate; +@property(nonatomic, weak) id<OmniboxGeolocationAuthorizationAlertDelegate> + delegate; // Designated initializer. Initializes this instance with |delegate|. - (instancetype)initWithDelegate:
diff --git a/ios/chrome/browser/geolocation/omnibox_geolocation_authorization_alert.mm b/ios/chrome/browser/geolocation/omnibox_geolocation_authorization_alert.mm index 0ce6d64..6fd9e75 100644 --- a/ios/chrome/browser/geolocation/omnibox_geolocation_authorization_alert.mm +++ b/ios/chrome/browser/geolocation/omnibox_geolocation_authorization_alert.mm
@@ -6,27 +6,24 @@ #import <UIKit/UIKit.h> -#import "base/ios/weak_nsobject.h" #include "base/logging.h" -#include "base/mac/scoped_nsobject.h" #include "components/strings/grit/components_strings.h" #include "ios/chrome/grit/ios_chromium_strings.h" #include "ios/public/provider/chrome/browser/chrome_browser_provider.h" #include "ui/base/l10n/l10n_util_mac.h" -@interface OmniboxGeolocationAuthorizationAlert () { - base::WeakNSProtocol<id<OmniboxGeolocationAuthorizationAlertDelegate>> - delegate_; -} -@end +#if !defined(__has_feature) || !__has_feature(objc_arc) +#error "This file requires ARC support." +#endif @implementation OmniboxGeolocationAuthorizationAlert +@synthesize delegate = _delegate; - (instancetype)initWithDelegate: (id<OmniboxGeolocationAuthorizationAlertDelegate>)delegate { self = [super init]; if (self) { - delegate_.reset(delegate); + _delegate = delegate; } return self; } @@ -35,14 +32,6 @@ return [self initWithDelegate:nil]; } -- (id<OmniboxGeolocationAuthorizationAlertDelegate>)delegate { - return delegate_.get(); -} - -- (void)setDelegate:(id<OmniboxGeolocationAuthorizationAlertDelegate>)delegate { - delegate_.reset(delegate); -} - - (void)showAuthorizationAlert { NSString* message = l10n_util::GetNSString(IDS_IOS_LOCATION_AUTHORIZATION_ALERT); @@ -51,7 +40,7 @@ // Use a UIAlertController to match the style of the iOS system location // alert. - base::WeakNSObject<OmniboxGeolocationAuthorizationAlert> weakSelf(self); + __weak OmniboxGeolocationAuthorizationAlert* weakSelf = self; UIAlertController* alert = [UIAlertController alertControllerWithTitle:nil message:message @@ -61,8 +50,7 @@ actionWithTitle:ok style:UIAlertActionStyleDefault handler:^(UIAlertAction* action) { - base::scoped_nsobject<OmniboxGeolocationAuthorizationAlert> - strongSelf([weakSelf retain]); + OmniboxGeolocationAuthorizationAlert* strongSelf = weakSelf; if (strongSelf) { [[strongSelf delegate] authorizationAlertDidAuthorize:strongSelf]; @@ -74,8 +62,7 @@ actionWithTitle:cancel style:UIAlertActionStyleCancel handler:^(UIAlertAction* action) { - base::scoped_nsobject<OmniboxGeolocationAuthorizationAlert> - strongSelf([weakSelf retain]); + OmniboxGeolocationAuthorizationAlert* strongSelf = weakSelf; if (strongSelf) { [[strongSelf delegate] authorizationAlertDidCancel:strongSelf];
diff --git a/ios/chrome/browser/geolocation/omnibox_geolocation_config.mm b/ios/chrome/browser/geolocation/omnibox_geolocation_config.mm index 8642497f..f411c1a 100644 --- a/ios/chrome/browser/geolocation/omnibox_geolocation_config.mm +++ b/ios/chrome/browser/geolocation/omnibox_geolocation_config.mm
@@ -12,6 +12,10 @@ #include "base/strings/sys_string_conversions.h" #include "url/gurl.h" +#if !defined(__has_feature) || !__has_feature(objc_arc) +#error "This file requires ARC support." +#endif + namespace { NSString* const kEligibleDomainsKey = @"EligibleDomains";
diff --git a/ios/chrome/browser/geolocation/omnibox_geolocation_local_state.mm b/ios/chrome/browser/geolocation/omnibox_geolocation_local_state.mm index c195f46f..82f8988 100644 --- a/ios/chrome/browser/geolocation/omnibox_geolocation_local_state.mm +++ b/ios/chrome/browser/geolocation/omnibox_geolocation_local_state.mm
@@ -7,15 +7,18 @@ #import <CoreLocation/CoreLocation.h> #include "base/logging.h" -#include "base/mac/scoped_nsobject.h" #include "components/prefs/pref_registry_simple.h" #include "components/prefs/pref_service.h" #include "ios/chrome/browser/application_context.h" #import "ios/chrome/browser/geolocation/location_manager.h" #import "ios/chrome/browser/pref_names.h" +#if !defined(__has_feature) || !__has_feature(objc_arc) +#error "This file requires ARC support." +#endif + @interface OmniboxGeolocationLocalState () { - base::scoped_nsobject<LocationManager> locationManager_; + LocationManager* _locationManager; } - (int)intForPath:(const char*)path; @@ -39,7 +42,7 @@ DCHECK(locationManager); self = [super init]; if (self) { - locationManager_.reset([locationManager retain]); + _locationManager = locationManager; } return self; } @@ -65,7 +68,7 @@ break; } - switch ([locationManager_ authorizationStatus]) { + switch ([_locationManager authorizationStatus]) { case kCLAuthorizationStatusNotDetermined: // If the user previously authorized or denied geolocation but reset the // system settings, then start over.
diff --git a/ios/chrome/browser/geolocation/test_location_manager.h b/ios/chrome/browser/geolocation/test_location_manager.h index b6de5fe..2bc84b59 100644 --- a/ios/chrome/browser/geolocation/test_location_manager.h +++ b/ios/chrome/browser/geolocation/test_location_manager.h
@@ -17,7 +17,7 @@ @property(nonatomic, assign) CLAuthorizationStatus authorizationStatus; // Writable version of the LocationManager |currentLocation| property. -@property(nonatomic, retain) CLLocation* currentLocation; +@property(nonatomic, strong) CLLocation* currentLocation; // Writable version of the LocationManager |locationServicesEnabled| property. @property(nonatomic, assign) BOOL locationServicesEnabled;
diff --git a/ios/chrome/browser/geolocation/test_location_manager.mm b/ios/chrome/browser/geolocation/test_location_manager.mm index eb6633e..8de6fff 100644 --- a/ios/chrome/browser/geolocation/test_location_manager.mm +++ b/ios/chrome/browser/geolocation/test_location_manager.mm
@@ -4,24 +4,20 @@ #import "ios/chrome/browser/geolocation/test_location_manager.h" -#include "base/mac/scoped_nsobject.h" +#if !defined(__has_feature) || !__has_feature(objc_arc) +#error "This file requires ARC support." +#endif -@interface TestLocationManager () { - CLAuthorizationStatus _authorizationStatus; - base::scoped_nsobject<CLLocation> _currentLocation; - BOOL _locationServicesEnabled; - BOOL _started; - BOOL _stopped; -} +@interface TestLocationManager () @end @implementation TestLocationManager - @synthesize authorizationStatus = _authorizationStatus; @synthesize locationServicesEnabled = _locationServicesEnabled; @synthesize started = _started; @synthesize stopped = _stopped; +@synthesize currentLocation = _currentLocation; - (id)init { self = [super init]; @@ -38,17 +34,9 @@ } } -- (CLLocation*)currentLocation { - return _currentLocation; -} - -- (void)setCurrentLocation:(CLLocation*)currentLocation { - _currentLocation.reset([currentLocation retain]); -} - - (void)reset { _authorizationStatus = kCLAuthorizationStatusNotDetermined; - _currentLocation.reset(); + _currentLocation = nil; _locationServicesEnabled = YES; _started = NO; _stopped = NO;
diff --git a/ios/chrome/browser/metrics/BUILD.gn b/ios/chrome/browser/metrics/BUILD.gn index 63b0e1b..e9a7abf 100644 --- a/ios/chrome/browser/metrics/BUILD.gn +++ b/ios/chrome/browser/metrics/BUILD.gn
@@ -3,6 +3,7 @@ # found in the LICENSE file. source_set("metrics") { + configs += [ "//build/config/compiler:enable_arc" ] sources = [ "field_trial_synchronizer.cc", "field_trial_synchronizer.h", @@ -57,6 +58,7 @@ } source_set("unit_tests") { + configs += [ "//build/config/compiler:enable_arc" ] testonly = true sources = [ "ios_chrome_metrics_service_accessor_unittest.cc",
diff --git a/ios/chrome/browser/metrics/ios_chrome_metrics_services_manager_client.mm b/ios/chrome/browser/metrics/ios_chrome_metrics_services_manager_client.mm index 869a4c9..e569339 100644 --- a/ios/chrome/browser/metrics/ios_chrome_metrics_services_manager_client.mm +++ b/ios/chrome/browser/metrics/ios_chrome_metrics_services_manager_client.mm
@@ -21,6 +21,10 @@ #include "ios/chrome/browser/variations/ios_ui_string_overrider_factory.h" #include "ios/public/provider/chrome/browser/chrome_browser_provider.h" +#if !defined(__has_feature) || !__has_feature(objc_arc) +#error "This file requires ARC support." +#endif + namespace { void PostStoreMetricsClientInfo(const metrics::ClientInfo& client_info) {}
diff --git a/ios/chrome/browser/metrics/mobile_session_shutdown_metrics_provider.mm b/ios/chrome/browser/metrics/mobile_session_shutdown_metrics_provider.mm index f13237ba..912ac2d 100644 --- a/ios/chrome/browser/metrics/mobile_session_shutdown_metrics_provider.mm +++ b/ios/chrome/browser/metrics/mobile_session_shutdown_metrics_provider.mm
@@ -13,6 +13,10 @@ #import "ios/chrome/browser/crash_report/crash_report_background_uploader.h" #import "ios/chrome/browser/metrics/previous_session_info.h" +#if !defined(__has_feature) || !__has_feature(objc_arc) +#error "This file requires ARC support." +#endif + namespace { // Logs |type| in the shutdown type histogram.
diff --git a/ios/chrome/browser/metrics/mobile_session_shutdown_metrics_provider_unittest.mm b/ios/chrome/browser/metrics/mobile_session_shutdown_metrics_provider_unittest.mm index 511b5e99..4c942ada 100644 --- a/ios/chrome/browser/metrics/mobile_session_shutdown_metrics_provider_unittest.mm +++ b/ios/chrome/browser/metrics/mobile_session_shutdown_metrics_provider_unittest.mm
@@ -20,6 +20,10 @@ #include "testing/gtest/include/gtest/gtest-param-test.h" #include "testing/gtest/include/gtest/gtest.h" +#if !defined(__has_feature) || !__has_feature(objc_arc) +#error "This file requires ARC support." +#endif + // An MobileSessionShutdownMetricsProvider that returns fake values for the last // session environment query methods. class MobileSessionShutdownMetricsProviderForTesting
diff --git a/ios/chrome/browser/metrics/previous_session_info.mm b/ios/chrome/browser/metrics/previous_session_info.mm index e435eae..30c4c4f 100644 --- a/ios/chrome/browser/metrics/previous_session_info.mm +++ b/ios/chrome/browser/metrics/previous_session_info.mm
@@ -5,11 +5,14 @@ #import "ios/chrome/browser/metrics/previous_session_info.h" #include "base/logging.h" -#include "base/mac/scoped_nsobject.h" #include "base/strings/sys_string_conversions.h" #include "components/version_info/version_info.h" #import "ios/chrome/browser/metrics/previous_session_info_private.h" +#if !defined(__has_feature) || !__has_feature(objc_arc) +#error "This file requires ARC support." +#endif + namespace { // Key in the NSUserDefaults for a string value that stores the version of the @@ -63,7 +66,6 @@ } + (void)resetSharedInstanceForTesting { - [gSharedInstance release]; gSharedInstance = nil; }
diff --git a/ios/chrome/browser/metrics/previous_session_info_unittest.mm b/ios/chrome/browser/metrics/previous_session_info_unittest.mm index da185be..5168ed0 100644 --- a/ios/chrome/browser/metrics/previous_session_info_unittest.mm +++ b/ios/chrome/browser/metrics/previous_session_info_unittest.mm
@@ -10,6 +10,10 @@ #include "testing/gtest/include/gtest/gtest.h" #include "testing/gtest_mac.h" +#if !defined(__has_feature) || !__has_feature(objc_arc) +#error "This file requires ARC support." +#endif + namespace { // Key in the UserDefaults for a boolean value keeping track of memory warnings.
diff --git a/ios/chrome/browser/native_app_launcher/BUILD.gn b/ios/chrome/browser/native_app_launcher/BUILD.gn index 95f06de..7c32d813 100644 --- a/ios/chrome/browser/native_app_launcher/BUILD.gn +++ b/ios/chrome/browser/native_app_launcher/BUILD.gn
@@ -37,7 +37,6 @@ "//base/test:test_support", "//components/infobars/core", "//ios/chrome/browser", - "//ios/chrome/browser/net:test_support", "//ios/chrome/test:test_support", "//ios/public/provider/chrome/browser", "//ios/public/provider/chrome/browser:test_support",
diff --git a/ios/chrome/browser/net/BUILD.gn b/ios/chrome/browser/net/BUILD.gn index b548450..ad7fb27 100644 --- a/ios/chrome/browser/net/BUILD.gn +++ b/ios/chrome/browser/net/BUILD.gn
@@ -15,8 +15,6 @@ "crl_set_fetcher.h", "http_server_properties_manager_factory.cc", "http_server_properties_manager_factory.h", - "image_fetcher.h", - "image_fetcher.mm", "ios_chrome_http_user_agent_settings.h", "ios_chrome_http_user_agent_settings.mm", "ios_chrome_network_delegate.cc", @@ -46,7 +44,6 @@ "//ios/chrome/browser/browsing_data", "//ios/net", "//ios/web", - "//ios/web/public/image_fetcher", "//net", "//net:extras", "//url", @@ -57,18 +54,6 @@ ] } -source_set("test_support") { - testonly = true - sources = [ - "mock_image_fetcher.h", - "mock_image_fetcher.mm", - ] - deps = [ - ":net", - "//testing/gmock", - ] -} - source_set("unit_tests") { configs += [ "//build/config/compiler:enable_arc" ] testonly = true
diff --git a/ios/chrome/browser/net/image_fetcher.h b/ios/chrome/browser/net/image_fetcher.h deleted file mode 100644 index 1f32077..0000000 --- a/ios/chrome/browser/net/image_fetcher.h +++ /dev/null
@@ -1,90 +0,0 @@ -// Copyright 2011 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#ifndef IOS_CHROME_BROWSER_NET_IMAGE_FETCHER_H_ -#define IOS_CHROME_BROWSER_NET_IMAGE_FETCHER_H_ - -#include <map> - -#include "base/mac/scoped_block.h" -#include "base/memory/ref_counted.h" -#include "base/memory/weak_ptr.h" -#include "net/url_request/url_fetcher_delegate.h" -#include "net/url_request/url_request.h" - -class GURL; -@class NSData; - -namespace base { -class TaskRunner; -} - -namespace net { -class URLRequestContextGetter; -} - -// Callback that informs of the download of an image encoded in |data|, -// downloaded from |url|, and with the http status |http_response_code|. If the -// url is a data URL, |http_response_code| is always 200. -using ImageFetchedCallback = - void (^)(const GURL& url, int http_response_code, NSData* data); - -// Utility class that will retrieve an image from an URL. The image is returned -// as NSData which can be used with +[UIImage imageWithData:]. This class -// usually returns the raw bytes retrieved from the network without any -// processing, with the exception of WebP encoded images. Those are decoded and -// then reencoded in a format suitable for UIImage. -// An instance of this class can download a number of images at the same time. -class ImageFetcher : public net::URLFetcherDelegate { - public: - // The TaskRunner is used to eventually decode the image. - explicit ImageFetcher(const scoped_refptr<base::TaskRunner>& task_runner); - ~ImageFetcher() override; - - // Start downloading the image at the given |url|. The |callback| will be - // called with the downloaded image, or nil if any error happened. The - // |referrer| and |referrer_policy| will be passed on to the underlying - // URLFetcher. - // This method assumes the request context getter has been set. - // (virtual for testing) - virtual void StartDownload(const GURL& url, - ImageFetchedCallback callback, - const std::string& referrer, - net::URLRequest::ReferrerPolicy referrer_policy); - - // Helper method to call StartDownload without a referrer. - // (virtual for testing) - virtual void StartDownload(const GURL& url, ImageFetchedCallback callback); - - // A valid request context getter is required before starting the download. - // (virtual for testing) - virtual void SetRequestContextGetter(const scoped_refptr< - net::URLRequestContextGetter>& request_context_getter); - - // net::URLFetcherDelegate: - void OnURLFetchComplete(const net::URLFetcher* source) override; - - private: - // Runs the callback with the given arguments. - void RunCallback(const base::mac::ScopedBlock<ImageFetchedCallback>& callback, - const GURL& url, - const int http_response_code, - NSData* data); - - // Tracks open download requests. The key is the URLFetcher object doing the - // fetch; the value is the callback to use when the download request - // completes. When a download request completes, the URLFetcher must be - // deleted and the callback called and released. - std::map<const net::URLFetcher*, ImageFetchedCallback> downloads_in_progress_; - scoped_refptr<net::URLRequestContextGetter> request_context_getter_; - - // The task runner used to decode images if necessary. - const scoped_refptr<base::TaskRunner> task_runner_; - - // The WeakPtrFactory is used to cancel callbacks if ImageFetcher is destroyed - // during WebP decoding. - base::WeakPtrFactory<ImageFetcher> weak_factory_; -}; - -#endif // IOS_CHROME_BROWSER_NET_IMAGE_FETCHER_H_
diff --git a/ios/chrome/browser/net/image_fetcher.mm b/ios/chrome/browser/net/image_fetcher.mm deleted file mode 100644 index 4ad0f02..0000000 --- a/ios/chrome/browser/net/image_fetcher.mm +++ /dev/null
@@ -1,177 +0,0 @@ -// Copyright 2012 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#import "ios/chrome/browser/net/image_fetcher.h" - -#import <Foundation/Foundation.h> -#include <stddef.h> - -#include "base/bind.h" -#include "base/location.h" -#include "base/mac/scoped_nsobject.h" -#include "base/task_runner.h" -#include "ios/web/public/image_fetcher/webp_decoder.h" -#include "ios/web/public/web_thread.h" -#include "net/base/load_flags.h" -#include "net/http/http_response_headers.h" -#include "net/url_request/url_fetcher.h" -#include "net/url_request/url_request_context_getter.h" - -#if !defined(__has_feature) || !__has_feature(objc_arc) -#error "This file requires ARC support." -#endif - -namespace { - -class WebpDecoderDelegate : public webp_transcode::WebpDecoder::Delegate { - public: - NSData* data() const { return decoded_image_; } - - // WebpDecoder::Delegate methods - void OnFinishedDecoding(bool success) override { - if (!success) - decoded_image_.reset(); - } - void SetImageFeatures( - size_t total_size, - webp_transcode::WebpDecoder::DecodedImageFormat format) override { - decoded_image_.reset([[NSMutableData alloc] initWithCapacity:total_size]); - } - void OnDataDecoded(NSData* data) override { - DCHECK(decoded_image_); - [decoded_image_ appendData:data]; - } - private: - ~WebpDecoderDelegate() override {} - base::scoped_nsobject<NSMutableData> decoded_image_; -}; - -// Content-type header for WebP images. -static const char kWEBPMimeType[] = "image/webp"; - -// Returns a NSData object containing the decoded image. -// Returns nil in case of failure. -base::scoped_nsobject<NSData> DecodeWebpImage( - const base::scoped_nsobject<NSData>& webp_image) { - scoped_refptr<WebpDecoderDelegate> delegate(new WebpDecoderDelegate); - scoped_refptr<webp_transcode::WebpDecoder> decoder( - new webp_transcode::WebpDecoder(delegate.get())); - decoder->OnDataReceived(webp_image); - DLOG_IF(ERROR, !delegate->data()) << "WebP image decoding failed."; - return base::scoped_nsobject<NSData>(delegate->data()); -} - -} // namespace - -ImageFetcher::ImageFetcher(const scoped_refptr<base::TaskRunner>& task_runner) - : request_context_getter_(nullptr), - task_runner_(task_runner), - weak_factory_(this) { - DCHECK(task_runner_.get()); -} - -ImageFetcher::~ImageFetcher() { - // Delete all the entries in the |downloads_in_progress_| map. This will in - // turn cancel all of the requests. - for (const auto& pair : downloads_in_progress_) { - delete pair.first; - } -} - -void ImageFetcher::StartDownload( - const GURL& url, - ImageFetchedCallback callback, - const std::string& referrer, - net::URLRequest::ReferrerPolicy referrer_policy) { - DCHECK(request_context_getter_.get()); - net::URLFetcher* fetcher = - net::URLFetcher::Create(url, net::URLFetcher::GET, this).release(); - downloads_in_progress_[fetcher] = [callback copy]; - fetcher->SetLoadFlags( - net::LOAD_DO_NOT_SEND_COOKIES | net::LOAD_DO_NOT_SAVE_COOKIES | - net::LOAD_DO_NOT_SEND_AUTH_DATA); - fetcher->SetRequestContext(request_context_getter_.get()); - fetcher->SetReferrer(referrer); - fetcher->SetReferrerPolicy(referrer_policy); - fetcher->Start(); -} - -void ImageFetcher::StartDownload( - const GURL& url, ImageFetchedCallback callback) { - ImageFetcher::StartDownload( - url, callback, std::string(), net::URLRequest::NEVER_CLEAR_REFERRER); -} - -// Delegate callback that is called when URLFetcher completes. If the image -// was fetched successfully, creates a new NSData and returns it to the -// callback, otherwise returns nil to the callback. -void ImageFetcher::OnURLFetchComplete(const net::URLFetcher* fetcher) { - if (downloads_in_progress_.find(fetcher) == downloads_in_progress_.end()) { - LOG(ERROR) << "Received callback for unknown URLFetcher " << fetcher; - return; - } - - // Ensures that |fetcher| will be deleted in the event of early return. - std::unique_ptr<const net::URLFetcher> fetcher_deleter(fetcher); - - // Retrieves the callback and ensures that it will be deleted in the event - // of early return. - base::mac::ScopedBlock<ImageFetchedCallback> callback( - downloads_in_progress_[fetcher]); - - // Remove |fetcher| from the map. - downloads_in_progress_.erase(fetcher); - - // Make sure the request was successful. For "data" requests, the response - // code has no meaning, because there is no actual server (data is encoded - // directly in the URL). In that case, set the response code to 200 (OK). - const GURL& original_url = fetcher->GetOriginalURL(); - const int http_response_code = original_url.SchemeIs("data") ? - 200 : fetcher->GetResponseCode(); - if (http_response_code != 200) { - (callback.get())(original_url, http_response_code, nil); - return; - } - - std::string response; - if (!fetcher->GetResponseAsString(&response)) { - (callback.get())(original_url, http_response_code, nil); - return; - } - - // Create a NSData from the returned data and notify the callback. - base::scoped_nsobject<NSData> data([[NSData alloc] - initWithBytes:reinterpret_cast<const unsigned char*>(response.data()) - length:response.size()]); - - if (fetcher->GetResponseHeaders()) { - std::string mime_type; - fetcher->GetResponseHeaders()->GetMimeType(&mime_type); - if (mime_type == kWEBPMimeType) { - base::PostTaskAndReplyWithResult(task_runner_.get(), - FROM_HERE, - base::Bind(&DecodeWebpImage, data), - base::Bind(&ImageFetcher::RunCallback, - weak_factory_.GetWeakPtr(), - callback, - original_url, - http_response_code)); - return; - } - } - (callback.get())(original_url, http_response_code, data); -} - -void ImageFetcher::RunCallback( - const base::mac::ScopedBlock<ImageFetchedCallback>& callback, - const GURL& url, - int http_response_code, - NSData* data) { - (callback.get())(url, http_response_code, data); -} - -void ImageFetcher::SetRequestContextGetter( - const scoped_refptr<net::URLRequestContextGetter>& request_context_getter) { - request_context_getter_ = request_context_getter; -}
diff --git a/ios/chrome/browser/net/mock_image_fetcher.h b/ios/chrome/browser/net/mock_image_fetcher.h deleted file mode 100644 index 259eda62..0000000 --- a/ios/chrome/browser/net/mock_image_fetcher.h +++ /dev/null
@@ -1,31 +0,0 @@ -// Copyright 2015 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#ifndef IOS_CHROME_BROWSER_NET_MOCK_IMAGE_FETCHER_H_ -#define IOS_CHROME_BROWSER_NET_MOCK_IMAGE_FETCHER_H_ - -#import "ios/chrome/browser/net/image_fetcher.h" - -#include "testing/gmock/include/gmock/gmock.h" - -// Mock the ImageFetcher utility class, which can be used to asynchronously -// retrieve an image from an URL. -class MockImageFetcher : public ImageFetcher { - public: - explicit MockImageFetcher(const scoped_refptr<base::TaskRunner>& task_runner); - ~MockImageFetcher() override; - - MOCK_METHOD4(StartDownload, - void(const GURL& url, - ImageFetchedCallback callback, - const std::string& referrer, - net::URLRequest::ReferrerPolicy referrer_policy)); - MOCK_METHOD2(StartDownload, - void(const GURL& url, ImageFetchedCallback callback)); - MOCK_METHOD1(SetRequestContextGetter, - void(const scoped_refptr<net::URLRequestContextGetter>& - request_context_getter)); -}; - -#endif // IOS_CHROME_BROWSER_NET_MOCK_IMAGE_FETCHER_H_
diff --git a/ios/chrome/browser/net/mock_image_fetcher.mm b/ios/chrome/browser/net/mock_image_fetcher.mm deleted file mode 100644 index 7b02b8c8..0000000 --- a/ios/chrome/browser/net/mock_image_fetcher.mm +++ /dev/null
@@ -1,13 +0,0 @@ -// Copyright 2015 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#import "ios/chrome/browser/net/mock_image_fetcher.h" - -MockImageFetcher::MockImageFetcher( - const scoped_refptr<base::TaskRunner>& task_runner) - : ImageFetcher(task_runner) { -} - -MockImageFetcher::~MockImageFetcher() { -}
diff --git a/ios/chrome/browser/open_from_clipboard/BUILD.gn b/ios/chrome/browser/open_from_clipboard/BUILD.gn index 585762a..0c4d8dc 100644 --- a/ios/chrome/browser/open_from_clipboard/BUILD.gn +++ b/ios/chrome/browser/open_from_clipboard/BUILD.gn
@@ -3,6 +3,7 @@ # found in the LICENSE file. source_set("open_from_clipboard") { + configs += [ "//build/config/compiler:enable_arc" ] sources = [ "create_clipboard_recent_content.h", "create_clipboard_recent_content.mm",
diff --git a/ios/chrome/browser/open_from_clipboard/create_clipboard_recent_content.mm b/ios/chrome/browser/open_from_clipboard/create_clipboard_recent_content.mm index 6ebb702..52f6cc78 100644 --- a/ios/chrome/browser/open_from_clipboard/create_clipboard_recent_content.mm +++ b/ios/chrome/browser/open_from_clipboard/create_clipboard_recent_content.mm
@@ -9,6 +9,10 @@ #include "ios/chrome/browser/chrome_url_constants.h" #include "ios/chrome/common/app_group/app_group_constants.h" +#if !defined(__has_feature) || !__has_feature(objc_arc) +#error "This file requires ARC support." +#endif + std::unique_ptr<ClipboardRecentContent> CreateClipboardRecentContentIOS() { return base::MakeUnique<ClipboardRecentContentIOS>( kChromeUIScheme, app_group::GetGroupUserDefaults());
diff --git a/ios/chrome/browser/ui/commands/BUILD.gn b/ios/chrome/browser/ui/commands/BUILD.gn index c02e52a..41a9f8c 100644 --- a/ios/chrome/browser/ui/commands/BUILD.gn +++ b/ios/chrome/browser/ui/commands/BUILD.gn
@@ -33,6 +33,7 @@ } source_set("unit_tests") { + configs += [ "//build/config/compiler:enable_arc" ] testonly = true sources = [ "set_up_for_testing_command_unittest.mm",
diff --git a/ios/chrome/browser/ui/commands/set_up_for_testing_command_unittest.mm b/ios/chrome/browser/ui/commands/set_up_for_testing_command_unittest.mm index 508f7bc..79ba5af 100644 --- a/ios/chrome/browser/ui/commands/set_up_for_testing_command_unittest.mm +++ b/ios/chrome/browser/ui/commands/set_up_for_testing_command_unittest.mm
@@ -4,20 +4,23 @@ #import "ios/chrome/browser/ui/commands/set_up_for_testing_command.h" -#include "base/mac/scoped_nsobject.h" #include "testing/gtest/include/gtest/gtest.h" #include "testing/gtest_mac.h" #include "testing/platform_test.h" #include "url/gurl.h" +#if !defined(__has_feature) || !__has_feature(objc_arc) +#error "This file requires ARC support." +#endif + namespace { typedef PlatformTest SetUpForTestingCommandTest; TEST_F(SetUpForTestingCommandTest, InitNoArguments) { GURL url("chrome://setupfortesting"); - base::scoped_nsobject<SetUpForTestingCommand> command( - [[SetUpForTestingCommand alloc] initWithURL:url]); + SetUpForTestingCommand* command = + [[SetUpForTestingCommand alloc] initWithURL:url]; EXPECT_FALSE([command clearBrowsingData]); EXPECT_FALSE([command closeTabs]); EXPECT_EQ(0, [command numberOfNewTabs]); @@ -25,8 +28,8 @@ TEST_F(SetUpForTestingCommandTest, InitClearBrowsingData) { GURL url("chrome://setupfortesting?clearBrowsingData"); - base::scoped_nsobject<SetUpForTestingCommand> command( - [[SetUpForTestingCommand alloc] initWithURL:url]); + SetUpForTestingCommand* command = + [[SetUpForTestingCommand alloc] initWithURL:url]; EXPECT_TRUE([command clearBrowsingData]); EXPECT_FALSE([command closeTabs]); EXPECT_EQ(0, [command numberOfNewTabs]); @@ -34,8 +37,8 @@ TEST_F(SetUpForTestingCommandTest, InitCloseTabs) { GURL url("chrome://setupfortesting?closeTabs"); - base::scoped_nsobject<SetUpForTestingCommand> command( - [[SetUpForTestingCommand alloc] initWithURL:url]); + SetUpForTestingCommand* command = + [[SetUpForTestingCommand alloc] initWithURL:url]; EXPECT_FALSE([command clearBrowsingData]); EXPECT_TRUE([command closeTabs]); EXPECT_EQ(0, [command numberOfNewTabs]); @@ -43,8 +46,8 @@ TEST_F(SetUpForTestingCommandTest, InitNumberOfNewTabs) { GURL url("chrome://setupfortesting?numberOfNewTabs=3"); - base::scoped_nsobject<SetUpForTestingCommand> command( - [[SetUpForTestingCommand alloc] initWithURL:url]); + SetUpForTestingCommand* command = + [[SetUpForTestingCommand alloc] initWithURL:url]; EXPECT_FALSE([command clearBrowsingData]); EXPECT_FALSE([command closeTabs]); EXPECT_EQ(3, [command numberOfNewTabs]); @@ -52,8 +55,8 @@ TEST_F(SetUpForTestingCommandTest, InitWithBadNumberOfNewTabs) { GURL url("chrome://setupfortesting?numberOfNewTabs=a"); - base::scoped_nsobject<SetUpForTestingCommand> command( - [[SetUpForTestingCommand alloc] initWithURL:url]); + SetUpForTestingCommand* command = + [[SetUpForTestingCommand alloc] initWithURL:url]; EXPECT_FALSE([command clearBrowsingData]); EXPECT_FALSE([command closeTabs]); EXPECT_EQ(0, [command numberOfNewTabs]); @@ -61,8 +64,8 @@ TEST_F(SetUpForTestingCommandTest, InitWithNegativeNumberOfNewTabs) { GURL url("chrome://setupfortesting?numberOfNewTabs=-3"); - base::scoped_nsobject<SetUpForTestingCommand> command( - [[SetUpForTestingCommand alloc] initWithURL:url]); + SetUpForTestingCommand* command = + [[SetUpForTestingCommand alloc] initWithURL:url]; EXPECT_FALSE([command clearBrowsingData]); EXPECT_FALSE([command closeTabs]); EXPECT_EQ(0, [command numberOfNewTabs]); @@ -71,8 +74,8 @@ TEST_F(SetUpForTestingCommandTest, InitWithArguments) { GURL url( "chrome://setupfortesting?clearBrowsingData&closeTabs&numberOfNewTabs=5"); - base::scoped_nsobject<SetUpForTestingCommand> command( - [[SetUpForTestingCommand alloc] initWithURL:url]); + SetUpForTestingCommand* command = + [[SetUpForTestingCommand alloc] initWithURL:url]; EXPECT_TRUE([command clearBrowsingData]); EXPECT_TRUE([command closeTabs]); EXPECT_EQ(5, [command numberOfNewTabs]); @@ -80,8 +83,8 @@ TEST_F(SetUpForTestingCommandTest, InitWithBadArguments) { GURL url("chrome://setupfortesting?badArg"); - base::scoped_nsobject<SetUpForTestingCommand> command( - [[SetUpForTestingCommand alloc] initWithURL:url]); + SetUpForTestingCommand* command = + [[SetUpForTestingCommand alloc] initWithURL:url]; EXPECT_FALSE([command clearBrowsingData]); EXPECT_FALSE([command closeTabs]); EXPECT_EQ(0, [command numberOfNewTabs]);
diff --git a/ios/chrome/browser/ui/voice/BUILD.gn b/ios/chrome/browser/ui/voice/BUILD.gn index d7602f2..1039524 100644 --- a/ios/chrome/browser/ui/voice/BUILD.gn +++ b/ios/chrome/browser/ui/voice/BUILD.gn
@@ -20,6 +20,7 @@ } source_set("unit_tests") { + configs += [ "//build/config/compiler:enable_arc" ] testonly = true sources = [
diff --git a/ios/chrome/browser/ui/voice/text_to_speech_player_unittest.mm b/ios/chrome/browser/ui/voice/text_to_speech_player_unittest.mm index 002e5521..8f6f203 100644 --- a/ios/chrome/browser/ui/voice/text_to_speech_player_unittest.mm +++ b/ios/chrome/browser/ui/voice/text_to_speech_player_unittest.mm
@@ -7,7 +7,6 @@ #import <UIKit/UIKit.h> #include "base/mac/foundation_util.h" -#import "base/mac/scoped_nsobject.h" #import "base/test/ios/wait_util.h" #include "base/time/time.h" #import "ios/chrome/browser/ui/voice/voice_search_notification_names.h" @@ -17,13 +16,17 @@ #include "testing/platform_test.h" #include "url/gurl.h" +#if !defined(__has_feature) || !__has_feature(objc_arc) +#error "This file requires ARC support." +#endif + #pragma mark - TTSPlayerObserver // Test object that listens for TTS notifications. @interface TTSPlayerObserver : NSObject // The TextToSpeechPlayer passed on initialization. -@property(nonatomic, retain) TextToSpeechPlayer* player; +@property(nonatomic, strong) TextToSpeechPlayer* player; // Whether notifications have been received. @property(nonatomic, readonly) BOOL readyNotificationReceived; @@ -37,23 +40,20 @@ @end -@implementation TTSPlayerObserver { - base::scoped_nsobject<TextToSpeechPlayer> _player; -} - +@implementation TTSPlayerObserver +@synthesize player = _player; @synthesize readyNotificationReceived = _readyNotificationReceived; @synthesize willStartNotificationReceived = _willStartNotificationReceived; @synthesize didStopNotificationReceived = _didStopNotificationReceived; - (void)dealloc { [[NSNotificationCenter defaultCenter] removeObserver:self]; - [super dealloc]; } - (void)setPlayer:(TextToSpeechPlayer*)player { NSNotificationCenter* defaultCenter = [NSNotificationCenter defaultCenter]; [defaultCenter removeObserver:self]; - _player.reset([player retain]); + _player = player; if (player) { [defaultCenter addObserver:self selector:@selector(handleReadyNotification:) @@ -96,21 +96,20 @@ class TextToSpeechPlayerTest : public PlatformTest { protected: void SetUp() override { - tts_player_.reset([[TextToSpeechPlayer alloc] init]); - tts_player_observer_.reset([[TTSPlayerObserver alloc] init]); + tts_player_ = [[TextToSpeechPlayer alloc] init]; + tts_player_observer_ = [[TTSPlayerObserver alloc] init]; [tts_player_observer_ setPlayer:tts_player_]; } - base::scoped_nsobject<TextToSpeechPlayer> tts_player_; - base::scoped_nsobject<TTSPlayerObserver> tts_player_observer_; + TextToSpeechPlayer* tts_player_; + TTSPlayerObserver* tts_player_observer_; web::TestWebThreadBundle web_thread_bundle_; }; // Tests that kTTSAudioReadyForPlaybackNotification is received and that // TTSPlayer state is updated. TEST_F(TextToSpeechPlayerTest, ReadyForPlayback) { - base::scoped_nsobject<NSData> audio_data( - [[@"audio_data" dataUsingEncoding:NSUTF8StringEncoding] retain]); + NSData* audio_data = [@"audio_data" dataUsingEncoding:NSUTF8StringEncoding]; [tts_player_ prepareToPlayAudioData:audio_data]; EXPECT_TRUE([tts_player_observer_ readyNotificationReceived]); EXPECT_TRUE([tts_player_ isReadyForPlayback]); @@ -119,8 +118,7 @@ // Tests that kTTSAudioReadyForPlaybackNotification is received and that // TTSPlayer's |-readyForPlayback| is NO for empty data. TEST_F(TextToSpeechPlayerTest, ReadyForPlaybackEmtpyData) { - base::scoped_nsobject<NSData> audio_data( - [[@"" dataUsingEncoding:NSUTF8StringEncoding] retain]); + NSData* audio_data = [@"" dataUsingEncoding:NSUTF8StringEncoding]; [tts_player_ prepareToPlayAudioData:audio_data]; EXPECT_TRUE([tts_player_observer_ readyNotificationReceived]); EXPECT_FALSE([tts_player_ isReadyForPlayback]); @@ -135,8 +133,7 @@ [[NSBundle mainBundle] pathForResource:@"test_sound" ofType:@"m4a" inDirectory:@"ios/chrome/test/data/voice"]; - base::scoped_nsobject<NSData> audio_data( - [[NSData alloc] initWithContentsOfFile:path]); + NSData* audio_data = [[NSData alloc] initWithContentsOfFile:path]; [tts_player_ prepareToPlayAudioData:audio_data]; [tts_player_ beginPlayback]; EXPECT_TRUE([tts_player_observer_ willStartNotificationReceived]); @@ -155,8 +152,7 @@ [[NSBundle mainBundle] pathForResource:@"test_sound" ofType:@"m4a" inDirectory:@"ios/chrome/test/data/voice"]; - base::scoped_nsobject<NSData> audio_data( - [[NSData alloc] initWithContentsOfFile:path]); + NSData* audio_data = [[NSData alloc] initWithContentsOfFile:path]; [tts_player_ prepareToPlayAudioData:audio_data]; [tts_player_ beginPlayback]; EXPECT_TRUE([tts_player_observer_ willStartNotificationReceived]);
diff --git a/ios/chrome/browser/voice/BUILD.gn b/ios/chrome/browser/voice/BUILD.gn index f40c064..d7c3926 100644 --- a/ios/chrome/browser/voice/BUILD.gn +++ b/ios/chrome/browser/voice/BUILD.gn
@@ -36,6 +36,7 @@ } source_set("tts") { + configs += [ "//build/config/compiler:enable_arc" ] sources = [ "text_to_speech_listener.h", "text_to_speech_listener.mm",
diff --git a/ios/chrome/browser/voice/text_to_speech_listener.mm b/ios/chrome/browser/voice/text_to_speech_listener.mm index f9a8468..29527e40 100644 --- a/ios/chrome/browser/voice/text_to_speech_listener.mm +++ b/ios/chrome/browser/voice/text_to_speech_listener.mm
@@ -6,9 +6,9 @@ #include <memory> -#include "base/ios/weak_nsobject.h" #include "base/logging.h" #include "base/mac/scoped_nsobject.h" +#include "base/memory/ptr_util.h" #include "ios/web/public/navigation_manager.h" #include "ios/web/public/web_state/web_state.h" #include "ios/web/public/web_state/web_state_observer.h" @@ -16,6 +16,10 @@ #import "ios/chrome/browser/voice/voice_search_url_rewriter.h" #include "url/gurl.h" +#if !defined(__has_feature) || !__has_feature(objc_arc) +#error "This file requires ARC support." +#endif + #pragma mark - TextToSpeechListener Private Interface class TextToSpeechWebStateObserver; @@ -23,7 +27,7 @@ @interface TextToSpeechListener () // The TextToSpeechListenerDelegate passed on initialization. -@property(nonatomic, readonly) id<TextToSpeechListenerDelegate> delegate; +@property(weak, nonatomic, readonly) id<TextToSpeechListenerDelegate> delegate; @end @@ -63,7 +67,7 @@ BOOL shouldParse = [listener_.delegate shouldTextToSpeechListener:listener_ parseDataFromURL:url]; if (shouldParse) { - base::WeakNSObject<TextToSpeechListener> weakListener(listener_); + __weak TextToSpeechListener* weakListener = listener_; ExtractVoiceSearchAudioDataFromWebState(web_state(), ^(NSData* audioData) { [[weakListener delegate] textToSpeechListener:weakListener didReceiveResult:audioData]; @@ -78,21 +82,20 @@ } #pragma mark - TextToSpeechListener - @implementation TextToSpeechListener { - // Backing object for property of the same name. - base::WeakNSProtocol<id<TextToSpeechListenerDelegate>> _delegate; // The TextToSpeechWebStateObserver that listens for Text-To-Speech data. std::unique_ptr<TextToSpeechWebStateObserver> _webStateObserver; } +@synthesize delegate = _delegate; - (instancetype)initWithWebState:(web::WebState*)webState delegate:(id<TextToSpeechListenerDelegate>)delegate { if ((self = [super init])) { DCHECK(webState); DCHECK(delegate); - _webStateObserver.reset(new TextToSpeechWebStateObserver(webState, self)); - _delegate.reset(delegate); + _webStateObserver = + base::MakeUnique<TextToSpeechWebStateObserver>(webState, self); + _delegate = delegate; } return self; } @@ -103,8 +106,4 @@ return _webStateObserver->web_state(); } -- (id<TextToSpeechListenerDelegate>)delegate { - return _delegate.get(); -} - @end
diff --git a/ios/chrome/browser/voice/text_to_speech_parser.mm b/ios/chrome/browser/voice/text_to_speech_parser.mm index ca7a5317..280a4c5c 100644 --- a/ios/chrome/browser/voice/text_to_speech_parser.mm +++ b/ios/chrome/browser/voice/text_to_speech_parser.mm
@@ -9,6 +9,10 @@ #include "ios/web/public/web_state/web_state.h" #import "third_party/google_toolbox_for_mac/src/Foundation/GTMStringEncoding.h" +#if !defined(__has_feature) || !__has_feature(objc_arc) +#error "This file requires ARC support." +#endif + namespace { // The start and end tags that delimit TTS audio data.
diff --git a/ios/chrome/browser/voice/voice_search_url_rewriter.mm b/ios/chrome/browser/voice/voice_search_url_rewriter.mm index 6887574..b7bdcbe 100644 --- a/ios/chrome/browser/voice/voice_search_url_rewriter.mm +++ b/ios/chrome/browser/voice/voice_search_url_rewriter.mm
@@ -16,6 +16,10 @@ #include "net/base/url_util.h" #include "url/gurl.h" +#if !defined(__has_feature) || !__has_feature(objc_arc) +#error "This file requires ARC support." +#endif + bool VoiceSearchURLRewriter(GURL* url, web::BrowserState* browser_state) { if (!google_util::IsGoogleSearchUrl(*url)) return false;
diff --git a/ios/consumer/base/debugger.mm b/ios/consumer/base/debugger.mm deleted file mode 100644 index ff2e2b3..0000000 --- a/ios/consumer/base/debugger.mm +++ /dev/null
@@ -1,14 +0,0 @@ -// Copyright 2013 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#include "base/debug/debugger.h" -#include "ios/public/consumer/base/debugger.h" - -namespace ios { - -bool BeingDebugged() { - return base::debug::BeingDebugged(); -} - -} // namespace ios
diff --git a/ios/public/consumer/base/BUILD.gn b/ios/public/consumer/base/BUILD.gn index eb83ba2..cc242cf 100644 --- a/ios/public/consumer/base/BUILD.gn +++ b/ios/public/consumer/base/BUILD.gn
@@ -3,8 +3,9 @@ # found in the LICENSE file. source_set("base") { + configs += [ "//build/config/compiler:enable_arc" ] sources = [ - "//ios/consumer/base/debugger.mm", + "//ios/consumer/base/debugger.cc", "//ios/public/consumer/base/debugger.h", ] deps = [
diff --git a/ios/testing/BUILD.gn b/ios/testing/BUILD.gn index bcaa4493b..9772939 100644 --- a/ios/testing/BUILD.gn +++ b/ios/testing/BUILD.gn
@@ -6,6 +6,7 @@ import("//testing/test.gni") source_set("ios_test_support") { + configs += [ "//build/config/compiler:enable_arc" ] testonly = true deps = [
diff --git a/ios/testing/wait_util.mm b/ios/testing/wait_util.mm index 794506d2..8b53628 100644 --- a/ios/testing/wait_util.mm +++ b/ios/testing/wait_util.mm
@@ -6,6 +6,10 @@ #include "base/test/ios/wait_util.h" +#if !defined(__has_feature) || !__has_feature(objc_arc) +#error "This file requires ARC support." +#endif + namespace testing { const NSTimeInterval kSpinDelaySeconds = 0.01;
diff --git a/media/audio/audio_output_controller.cc b/media/audio/audio_output_controller.cc index c8a75b6..55ce9502 100644 --- a/media/audio/audio_output_controller.cc +++ b/media/audio/audio_output_controller.cc
@@ -138,14 +138,14 @@ audio_manager_->MakeAudioOutputStreamProxy(params_, output_device_id_); if (!stream_) { state_ = kError; - handler_->OnError(); + handler_->OnControllerError(); return; } if (!stream_->Open()) { DoStopCloseAndClearStream(); state_ = kError; - handler_->OnError(); + handler_->OnControllerError(); return; } @@ -162,7 +162,7 @@ // And then report we have been created if we haven't done so already. if (!is_for_device_change) - handler_->OnCreated(); + handler_->OnControllerCreated(); } void AudioOutputController::DoPlay() { @@ -197,7 +197,7 @@ FROM_HERE, TimeDelta::FromSeconds(5), this, &AudioOutputController::WedgeCheck); - handler_->OnPlaying(); + handler_->OnControllerPlaying(); } void AudioOutputController::StopStream() { @@ -230,7 +230,7 @@ // a better way to know when it should exit PPB_Audio_Shared::Run(). sync_reader_->RequestMoreData(base::TimeDelta::Max(), base::TimeTicks(), 0); - handler_->OnPaused(); + handler_->OnControllerPaused(); } void AudioOutputController::DoClose() { @@ -291,7 +291,7 @@ void AudioOutputController::DoReportError() { DCHECK(message_loop_->BelongsToCurrentThread()); if (state_ != kClosed) - handler_->OnError(); + handler_->OnControllerError(); } int AudioOutputController::OnMoreData(base::TimeDelta delay,
diff --git a/media/audio/audio_output_controller.h b/media/audio/audio_output_controller.h index ce16c1d1..f3d4b428 100644 --- a/media/audio/audio_output_controller.h +++ b/media/audio/audio_output_controller.h
@@ -71,10 +71,10 @@ // following methods are called on the audio manager thread. class MEDIA_EXPORT EventHandler { public: - virtual void OnCreated() = 0; - virtual void OnPlaying() = 0; - virtual void OnPaused() = 0; - virtual void OnError() = 0; + virtual void OnControllerCreated() = 0; + virtual void OnControllerPlaying() = 0; + virtual void OnControllerPaused() = 0; + virtual void OnControllerError() = 0; protected: virtual ~EventHandler() {} @@ -108,8 +108,8 @@ // Factory method for creating an AudioOutputController. // This also creates and opens an AudioOutputStream on the audio manager // thread, and if this is successful, the |event_handler| will receive an - // OnCreated() call from the same audio manager thread. |audio_manager| must - // outlive AudioOutputController. + // OnControllerCreated() call from the same audio manager thread. + // |audio_manager| must outlive AudioOutputController. // The |output_device_id| can be either empty (default device) or specify a // specific hardware device for audio output. static scoped_refptr<AudioOutputController> Create( @@ -136,9 +136,10 @@ void Pause(); // Closes the audio output stream. The state is changed and the resources - // are freed on the audio manager thread. closed_task is executed after that. - // Callbacks (EventHandler and SyncReader) must exist until closed_task is - // called. + // are freed on the audio manager thread. |closed_task| is executed after + // that, on the thread on which Close was called. Callbacks (EventHandler and + // SyncReader) must exist until closed_task is called, but they are safe + // to delete after that. // // It is safe to call this method more than once. Calls after the first one // will have no effect.
diff --git a/media/audio/audio_output_controller_unittest.cc b/media/audio/audio_output_controller_unittest.cc index 0fc47c82..5b6d883 100644 --- a/media/audio/audio_output_controller_unittest.cc +++ b/media/audio/audio_output_controller_unittest.cc
@@ -48,10 +48,10 @@ public: MockAudioOutputControllerEventHandler() {} - MOCK_METHOD0(OnCreated, void()); - MOCK_METHOD0(OnPlaying, void()); - MOCK_METHOD0(OnPaused, void()); - MOCK_METHOD0(OnError, void()); + MOCK_METHOD0(OnControllerCreated, void()); + MOCK_METHOD0(OnControllerPlaying, void()); + MOCK_METHOD0(OnControllerPaused, void()); + MOCK_METHOD0(OnControllerError, void()); private: DISALLOW_COPY_AND_ASSIGN(MockAudioOutputControllerEventHandler); @@ -125,7 +125,7 @@ kSampleRate, kBitsPerSample, samples_per_packet); if (params_.IsValid()) { - EXPECT_CALL(mock_event_handler_, OnCreated()); + EXPECT_CALL(mock_event_handler_, OnControllerCreated()); } controller_ = AudioOutputController::Create( @@ -139,8 +139,8 @@ } void Play() { - // Expect the event handler to receive one OnPlaying() call. - EXPECT_CALL(mock_event_handler_, OnPlaying()); + // Expect the event handler to receive one OnControllerPlaying() call. + EXPECT_CALL(mock_event_handler_, OnControllerPlaying()); // During playback, the mock pretends to provide audio data rendered and // sent from the render process. @@ -151,19 +151,18 @@ } void Pause() { - // Expect the event handler to receive one OnPaused() call. - EXPECT_CALL(mock_event_handler_, OnPaused()); + // Expect the event handler to receive one OnControllerPaused() call. + EXPECT_CALL(mock_event_handler_, OnControllerPaused()); controller_->Pause(); base::RunLoop().RunUntilIdle(); } void ChangeDevice() { - // Expect the event handler to receive one OnPaying() call and no OnPaused() - // call. - EXPECT_CALL(mock_event_handler_, OnPlaying()); - EXPECT_CALL(mock_event_handler_, OnPaused()) - .Times(0); + // Expect the event handler to receive one OnControllerPaying() call and no + // OnControllerPaused() call. + EXPECT_CALL(mock_event_handler_, OnControllerPlaying()); + EXPECT_CALL(mock_event_handler_, OnControllerPaused()).Times(0); // Simulate a device change event to AudioOutputController from the // AudioManager. @@ -174,9 +173,9 @@ void Divert(bool was_playing, int num_times_to_be_started) { if (was_playing) { - // Expect the handler to receive one OnPlaying() call as a result of the - // stream switching. - EXPECT_CALL(mock_event_handler_, OnPlaying()); + // Expect the handler to receive one OnControllerPlaying() call as a + // result of the stream switching. + EXPECT_CALL(mock_event_handler_, OnControllerPlaying()); } EXPECT_CALL(mock_stream_, Open()) @@ -228,9 +227,9 @@ void Revert(bool was_playing) { if (was_playing) { - // Expect the handler to receive one OnPlaying() call as a result of the - // stream switching back. - EXPECT_CALL(mock_event_handler_, OnPlaying()); + // Expect the handler to receive one OnControllerPlaying() call as a + // result of the stream switching back. + EXPECT_CALL(mock_event_handler_, OnControllerPlaying()); } EXPECT_CALL(mock_stream_, Close()); @@ -250,7 +249,7 @@ // Expect the current stream to close and a new stream to start // playing if not diverting. When diverting, nothing happens // until diverting is stopped. - EXPECT_CALL(mock_event_handler_, OnPlaying()); + EXPECT_CALL(mock_event_handler_, OnControllerPlaying()); } controller_->SwitchOutputDevice(
diff --git a/media/audio/audio_streams_tracker.cc b/media/audio/audio_streams_tracker.cc index 97b01ca2..5363796 100644 --- a/media/audio/audio_streams_tracker.cc +++ b/media/audio/audio_streams_tracker.cc
@@ -4,6 +4,8 @@ #include "media/audio/audio_streams_tracker.h" +#include <limits> + namespace media { AudioStreamsTracker::AudioStreamsTracker() @@ -17,16 +19,16 @@ void AudioStreamsTracker::IncreaseStreamCount() { DCHECK(thread_checker_.CalledOnValidThread()); - DCHECK_NE(current_stream_count_, SIZE_MAX); + DCHECK_NE(current_stream_count_, std::numeric_limits<size_t>::max()); ++current_stream_count_; if (current_stream_count_ > max_stream_count_) max_stream_count_ = current_stream_count_; } -void AudioStreamsTracker::DecreaseStreamCount() { +void AudioStreamsTracker::DecreaseStreamCount(size_t count) { DCHECK(thread_checker_.CalledOnValidThread()); - DCHECK_NE(current_stream_count_, 0u); - --current_stream_count_; + DCHECK_GE(current_stream_count_, count); + current_stream_count_ -= count; } void AudioStreamsTracker::ResetMaxStreamCount() {
diff --git a/media/audio/audio_streams_tracker.h b/media/audio/audio_streams_tracker.h index f501a218..1071fd7 100644 --- a/media/audio/audio_streams_tracker.h +++ b/media/audio/audio_streams_tracker.h
@@ -24,7 +24,7 @@ // Increases/decreases current stream count. Updates max stream count if // current count is larger. void IncreaseStreamCount(); - void DecreaseStreamCount(); + void DecreaseStreamCount(size_t count = 1); // Resets the max stream count, i.e. sets it to the current stream count. void ResetMaxStreamCount();
diff --git a/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/cross-origin-preflight-get-response-type-blob.html b/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/cross-origin-preflight-get-response-type-blob.html index 1919469..037ffba 100644 --- a/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/cross-origin-preflight-get-response-type-blob.html +++ b/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/cross-origin-preflight-get-response-type-blob.html
@@ -21,17 +21,23 @@ xhr.open("GET", "http://localhost:8000/xmlhttprequest/resources/cross-origin-preflight-get.php", true); // Make this a "non-simple" cross-origin request by adding a custom header. xhr.setRequestHeader("X-Proprietary-Header", "foo"); - xhr.onerror = function() { log("onerror") } + xhr.onerror = function() { + log("xhr onerror"); + if (window.testRunner) + testRunner.notifyDone(); + } xhr.onload = function() { var reader = new FileReader; reader.onload = function(evt) { log(evt.target.result); + }; + reader.onerror = () => { + log("reader.onerror"); + }; + reader.onloadend = () => { if (window.testRunner) testRunner.notifyDone(); }; - reader.onerror = function () { - log("reader.onerror"); - } reader.readAsText(xhr.response); } xhr.send(null);
diff --git a/third_party/WebKit/Source/core/css/parser/CSSLazyParsingState.cpp b/third_party/WebKit/Source/core/css/parser/CSSLazyParsingState.cpp index 8370f0a..1a231c6 100644 --- a/third_party/WebKit/Source/core/css/parser/CSSLazyParsingState.cpp +++ b/third_party/WebKit/Source/core/css/parser/CSSLazyParsingState.cpp
@@ -3,8 +3,10 @@ // found in the LICENSE file. #include "core/css/parser/CSSLazyParsingState.h" +#include "core/css/parser/CSSLazyPropertyParserImpl.h" #include "core/css/parser/CSSParserTokenRange.h" #include "core/frame/UseCounter.h" +#include "platform/Histogram.h" namespace blink { @@ -15,7 +17,19 @@ : m_context(context), m_escapedStrings(std::move(escapedStrings)), m_sheetText(sheetText), - m_owningContents(contents) {} + m_owningContents(contents), + m_parsedStyleRules(0), + m_totalStyleRules(0), + m_styleRulesNeededForNextMilestone(0), + m_usage(UsageGe0) { + recordUsageMetrics(); +} + +CSSLazyPropertyParserImpl* CSSLazyParsingState::createLazyParser( + const CSSParserTokenRange& block) { + ++m_totalStyleRules; + return new CSSLazyPropertyParserImpl(std::move(block), this); +} const CSSParserContext& CSSLazyParsingState::context() { DCHECK(m_owningContents); @@ -25,9 +39,18 @@ return m_context; } +void CSSLazyParsingState::countRuleParsed() { + ++m_parsedStyleRules; + while (m_parsedStyleRules > m_styleRulesNeededForNextMilestone) { + DCHECK_NE(UsageAll, m_usage); + ++m_usage; + recordUsageMetrics(); + } +} + bool CSSLazyParsingState::shouldLazilyParseProperties( const CSSSelectorList& selectors, - const CSSParserTokenRange& block) { + const CSSParserTokenRange& block) const { // Simple heuristic for an empty block. Note that |block| here does not // include {} brackets. We avoid lazy parsing empty blocks so we can avoid // considering them when possible for matching. Lazy blocks must always be @@ -53,4 +76,34 @@ return true; } +void CSSLazyParsingState::recordUsageMetrics() { + DEFINE_STATIC_LOCAL(EnumerationHistogram, usageHistogram, + ("Style.LazyUsage.Percent", UsageLastValue)); + switch (m_usage) { + case UsageGe0: + m_styleRulesNeededForNextMilestone = m_totalStyleRules * .1; + break; + case UsageGt10: + m_styleRulesNeededForNextMilestone = m_totalStyleRules * .25; + break; + case UsageGt25: + m_styleRulesNeededForNextMilestone = m_totalStyleRules * .5; + break; + case UsageGt50: + m_styleRulesNeededForNextMilestone = m_totalStyleRules * .75; + break; + case UsageGt75: + m_styleRulesNeededForNextMilestone = m_totalStyleRules * .9; + break; + case UsageGt90: + m_styleRulesNeededForNextMilestone = m_totalStyleRules - 1; + break; + case UsageAll: + m_styleRulesNeededForNextMilestone = m_totalStyleRules; + break; + } + + usageHistogram.count(m_usage); +} + } // namespace blink
diff --git a/third_party/WebKit/Source/core/css/parser/CSSLazyParsingState.h b/third_party/WebKit/Source/core/css/parser/CSSLazyParsingState.h index 3234141..8c77cb7 100644 --- a/third_party/WebKit/Source/core/css/parser/CSSLazyParsingState.h +++ b/third_party/WebKit/Source/core/css/parser/CSSLazyParsingState.h
@@ -13,6 +13,7 @@ namespace blink { +class CSSLazyPropertyParserImpl; class CSSParserTokenRange; // This class helps lazy parsing by retaining necessary state. It should not @@ -26,14 +27,36 @@ const String& sheetText, StyleSheetContents*); + // Helper method used to bump m_totalStyleRules. + CSSLazyPropertyParserImpl* createLazyParser(const CSSParserTokenRange& block); + const CSSParserContext& context(); + void countRuleParsed(); + bool shouldLazilyParseProperties(const CSSSelectorList&, - const CSSParserTokenRange& block); + const CSSParserTokenRange& block) const; DEFINE_INLINE_TRACE() { visitor->trace(m_owningContents); } + // Exposed for tests. This enum is used to back a histogram, so new values + // must be appended to the end, before UsageLastValue. + enum CSSRuleUsage { + UsageGe0 = 0, + UsageGt10 = 1, + UsageGt25 = 2, + UsageGt50 = 3, + UsageGt75 = 4, + UsageGt90 = 5, + UsageAll = 6, + + // This value must be last. + UsageLastValue = 7 + }; + private: + void recordUsageMetrics(); + CSSParserContext m_context; Vector<String> m_escapedStrings; // Also referenced on the css resource. @@ -42,6 +65,14 @@ // Weak to ensure lazy state will never cause the contents to live longer than // it should (we DCHECK this fact). WeakMember<StyleSheetContents> m_owningContents; + + // Used for calculating the % of rules that ended up being parsed. + int m_parsedStyleRules; + int m_totalStyleRules; + + int m_styleRulesNeededForNextMilestone; + + int m_usage; }; } // namespace blink
diff --git a/third_party/WebKit/Source/core/css/parser/CSSLazyParsingTest.cpp b/third_party/WebKit/Source/core/css/parser/CSSLazyParsingTest.cpp index a51f681..8adfe18 100644 --- a/third_party/WebKit/Source/core/css/parser/CSSLazyParsingTest.cpp +++ b/third_party/WebKit/Source/core/css/parser/CSSLazyParsingTest.cpp
@@ -5,11 +5,13 @@ #include "core/css/CSSStyleSheet.h" #include "core/css/StyleRule.h" #include "core/css/StyleSheetContents.h" +#include "core/css/parser/CSSLazyParsingState.h" #include "core/css/parser/CSSParser.h" #include "core/css/parser/CSSParserMode.h" #include "core/frame/FrameHost.h" #include "core/testing/DummyPageHolder.h" #include "platform/heap/Heap.h" +#include "platform/testing/HistogramTester.h" #include "testing/gtest/include/gtest/gtest.h" #include "wtf/text/WTFString.h" @@ -24,6 +26,9 @@ StyleRule* ruleAt(StyleSheetContents* sheet, size_t index) { return toStyleRule(sheet->childRules()[index]); } + + protected: + HistogramTester m_histogramTester; }; TEST_F(CSSLazyParsingTest, Simple) { @@ -152,4 +157,52 @@ EXPECT_FALSE(useCounter1.isCounted(CSSPropertyColor)); } +TEST_F(CSSLazyParsingTest, SimpleRuleUsagePercent) { + CSSParserContext context(HTMLStandardMode, nullptr); + StyleSheetContents* styleSheet = StyleSheetContents::create(context); + + std::string metricName = "Style.LazyUsage.Percent"; + m_histogramTester.expectTotalCount(metricName, 0); + + String sheetText = + "body { background-color: red; }" + "p { color: blue; }" + "a { color: yellow; }" + "#id { color: blue; }" + "div { color: grey; }"; + CSSParser::parseSheet(context, styleSheet, sheetText, true /* lazy parse */); + + m_histogramTester.expectTotalCount(metricName, 1); + m_histogramTester.expectUniqueSample(metricName, + CSSLazyParsingState::UsageGe0, 1); + + ruleAt(styleSheet, 0)->properties(); + m_histogramTester.expectTotalCount(metricName, 2); + m_histogramTester.expectBucketCount(metricName, + CSSLazyParsingState::UsageGt10, 1); + + ruleAt(styleSheet, 1)->properties(); + m_histogramTester.expectTotalCount(metricName, 3); + m_histogramTester.expectBucketCount(metricName, + CSSLazyParsingState::UsageGt25, 1); + + ruleAt(styleSheet, 2)->properties(); + m_histogramTester.expectTotalCount(metricName, 4); + m_histogramTester.expectBucketCount(metricName, + CSSLazyParsingState::UsageGt50, 1); + + ruleAt(styleSheet, 3)->properties(); + m_histogramTester.expectTotalCount(metricName, 5); + m_histogramTester.expectBucketCount(metricName, + CSSLazyParsingState::UsageGt75, 1); + + // Parsing the last rule bumps both Gt90 and All buckets. + ruleAt(styleSheet, 4)->properties(); + m_histogramTester.expectTotalCount(metricName, 7); + m_histogramTester.expectBucketCount(metricName, + CSSLazyParsingState::UsageGt90, 1); + m_histogramTester.expectBucketCount(metricName, CSSLazyParsingState::UsageAll, + 1); +} + } // namespace blink
diff --git a/third_party/WebKit/Source/core/css/parser/CSSLazyPropertyParserImpl.cpp b/third_party/WebKit/Source/core/css/parser/CSSLazyPropertyParserImpl.cpp index b075466f..7431d11 100644 --- a/third_party/WebKit/Source/core/css/parser/CSSLazyPropertyParserImpl.cpp +++ b/third_party/WebKit/Source/core/css/parser/CSSLazyPropertyParserImpl.cpp
@@ -19,6 +19,7 @@ } StylePropertySet* CSSLazyPropertyParserImpl::parseProperties() { + m_lazyState->countRuleParsed(); return CSSParserImpl::parseDeclarationListForLazyStyle( m_tokens, m_lazyState->context()); }
diff --git a/third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp b/third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp index b871809..8140d02 100644 --- a/third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp +++ b/third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp
@@ -801,9 +801,8 @@ } else if (m_lazyState && m_lazyState->shouldLazilyParseProperties(selectorList, block)) { DCHECK(m_styleSheet); - return StyleRule::createLazy( - std::move(selectorList), - new CSSLazyPropertyParserImpl(block, m_lazyState)); + return StyleRule::createLazy(std::move(selectorList), + m_lazyState->createLazyParser(block)); } consumeDeclarationList(block, StyleRule::Style);
diff --git a/third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp b/third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp index eef9c62..5b5625d 100644 --- a/third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp +++ b/third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp
@@ -1216,8 +1216,11 @@ const CSPDirectiveListVector& policies) { SourceListDirectiveVector sourceListDirectives; for (const auto& policy : policies) { - if (SourceListDirective* directive = policy->operativeDirective(type)) + if (SourceListDirective* directive = policy->operativeDirective(type)) { + if (directive->isNone()) + return SourceListDirectiveVector(1, directive); sourceListDirectives.append(directive); + } } return sourceListDirectives; @@ -1248,7 +1251,7 @@ // Embedding-CSP. SourceListDirectiveVector requiredList = getSourceVector(directive, CSPDirectiveListVector(1, this)); - if (requiredList.size() == 0) + if (!requiredList.size()) continue; SourceListDirective* required = requiredList[0]; // Aggregate all serialized source lists of the returned CSP into a vector
diff --git a/third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp b/third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp index 48c4aaee..b0ba8da 100644 --- a/third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp +++ b/third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp
@@ -18,6 +18,11 @@ public: CSPDirectiveListTest() : csp(ContentSecurityPolicy::create()) {} + virtual void SetUp() { + csp->setupSelf( + *SecurityOrigin::createFromString("https://example.test/image.png")); + } + CSPDirectiveList* createList(const String& list, ContentSecurityPolicyHeaderType type) { Vector<UChar> characters; @@ -516,6 +521,106 @@ } } +TEST_F(CSPDirectiveListTest, SubsumesIfNoneIsPresent) { + struct TestCase { + const char* policyA; + const std::vector<const char*> policiesB; + bool expected; + } cases[] = { + // `policyA` subsumes any vector of policies. + {"", {""}, true}, + {"", {"script-src http://example.com"}, true}, + {"", {"script-src 'none'"}, true}, + {"", {"script-src http://*.one.com", "script-src https://two.com"}, true}, + // `policyA` is 'none', but no policy in `policiesB` is. + {"script-src ", {""}, false}, + {"script-src 'none'", {""}, false}, + {"script-src ", {"script-src http://example.com"}, false}, + {"script-src 'none'", {"script-src http://example.com"}, false}, + {"script-src ", {"img-src 'none'"}, false}, + {"script-src 'none'", {"img-src 'none'"}, false}, + {"script-src ", + {"script-src http://*.one.com", "img-src https://two.com"}, + false}, + {"script-src 'none'", + {"script-src http://*.one.com", "img-src https://two.com"}, + false}, + {"script-src 'none'", + {"script-src http://*.one.com", "script-src https://two.com"}, + true}, + {"script-src 'none'", + {"script-src http://*.one.com", "script-src 'self'"}, + true}, + // `policyA` is not 'none', but at least effective result of `policiesB` + // is. + {"script-src http://example.com 'none'", {"script-src 'none'"}, true}, + {"script-src http://example.com", {"script-src 'none'"}, true}, + {"script-src http://example.com 'none'", + {"script-src http://*.one.com", "script-src http://one.com", + "script-src 'none'"}, + true}, + {"script-src http://example.com", + {"script-src http://*.one.com", "script-src http://one.com", + "script-src 'none'"}, + true}, + {"script-src http://one.com 'none'", + {"script-src http://*.one.com", "script-src http://one.com", + "script-src https://one.com"}, + true}, + // `policyA` is `none` and at least effective result of `policiesB` is + // too. + {"script-src ", {"script-src ", "script-src "}, true}, + {"script-src 'none'", {"script-src", "script-src 'none'"}, true}, + {"script-src ", {"script-src 'none'", "script-src 'none'"}, true}, + {"script-src ", + {"script-src 'none' http://example.com", + "script-src 'none' http://example.com"}, + false}, + {"script-src 'none'", {"script-src 'none'", "script-src 'none'"}, true}, + {"script-src 'none'", + {"script-src 'none'", "script-src 'none'", "script-src 'none'"}, + true}, + {"script-src 'none'", + {"script-src http://*.one.com", "script-src http://one.com", + "script-src 'none'"}, + true}, + {"script-src 'none'", + {"script-src http://*.one.com", "script-src http://two.com", + "script-src http://three.com"}, + true}, + // Policies contain special keywords. + {"script-src ", {"script-src ", "script-src 'unsafe-eval'"}, true}, + {"script-src 'none'", + {"script-src 'unsafe-inline'", "script-src 'none'"}, + true}, + {"script-src ", + {"script-src 'none' 'unsafe-inline'", + "script-src 'none' 'unsafe-inline'"}, + false}, + {"script-src ", + {"script-src 'none' 'unsafe-inline'", + "script-src 'unsafe-inline' 'strict-dynamic'"}, + false}, + {"script-src 'unsafe-eval'", + {"script-src 'unsafe-eval'", "script 'unsafe-inline'"}, + true}, + {"script-src 'unsafe-inline'", + {"script-src ", "script http://example.com"}, + true}, + }; + + for (const auto& test : cases) { + CSPDirectiveList* A = + createList(test.policyA, ContentSecurityPolicyHeaderTypeEnforce); + + HeapVector<Member<CSPDirectiveList>> listB; + for (const auto& policyB : test.policiesB) + listB.append(createList(policyB, ContentSecurityPolicyHeaderTypeEnforce)); + + EXPECT_EQ(test.expected, A->subsumes(listB)); + } +} + TEST_F(CSPDirectiveListTest, OperativeDirectiveGivenType) { enum DefaultBehaviour { Default, NoDefault, ChildAndDefault };
diff --git a/third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp b/third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp index 49fc73f..6449cf95 100644 --- a/third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp +++ b/third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp
@@ -103,6 +103,12 @@ return m_allowHashedAttributes; } +bool SourceListDirective::isNone() const { + return !m_list.size() && !m_allowSelf && !m_allowStar && !m_allowInline && + !m_allowHashedAttributes && !m_allowEval && !m_allowDynamic && + !m_nonces.size() && !m_hashes.size(); +} + uint8_t SourceListDirective::hashAlgorithmsUsed() const { return m_hashAlgorithmsUsed; } @@ -595,8 +601,8 @@ bool SourceListDirective::subsumes( HeapVector<Member<SourceListDirective>> other) { // TODO(amalika): Handle here special keywords. - if (!m_list.size() || !other.size()) - return !m_list.size(); + if (!other.size() || other[0]->isNone()) + return other.size(); HeapVector<Member<CSPSource>> normalizedA = m_list; if (m_allowSelf && other[0]->m_policy->getSelfSource())
diff --git a/third_party/WebKit/Source/core/frame/csp/SourceListDirective.h b/third_party/WebKit/Source/core/frame/csp/SourceListDirective.h index fb105657..fd4aa311 100644 --- a/third_party/WebKit/Source/core/frame/csp/SourceListDirective.h +++ b/third_party/WebKit/Source/core/frame/csp/SourceListDirective.h
@@ -43,6 +43,7 @@ bool allowNonce(const String& nonce) const; bool allowHash(const CSPHashValue&) const; bool allowHashedAttributes() const; + bool isNone() const; bool isHashOrNoncePresent() const; uint8_t hashAlgorithmsUsed() const; bool allowAllInline();
diff --git a/third_party/WebKit/Source/core/frame/csp/SourceListDirectiveTest.cpp b/third_party/WebKit/Source/core/frame/csp/SourceListDirectiveTest.cpp index 84a605a..0e1fcc3 100644 --- a/third_party/WebKit/Source/core/frame/csp/SourceListDirectiveTest.cpp +++ b/third_party/WebKit/Source/core/frame/csp/SourceListDirectiveTest.cpp
@@ -438,12 +438,6 @@ } EXPECT_EQ(required.subsumes(returned), test.expected); - - // If required is empty, any returned should be subsumed by it. - SourceListDirective requiredIsEmpty("script-src", "", csp.get()); - EXPECT_TRUE( - requiredIsEmpty.subsumes(HeapVector<Member<SourceListDirective>>())); - EXPECT_TRUE(requiredIsEmpty.subsumes(returned)); } } @@ -819,4 +813,42 @@ } } +TEST_F(SourceListDirectiveTest, IsNone) { + struct TestCase { + String sources; + bool expected; + } cases[] = { + // Source list is 'none'. + {"'none'", true}, + {"", true}, + {" ", true}, + // Source list is not 'none'. + {"http://example1.com/foo/", false}, + {"'sha512-321cba'", false}, + {"'nonce-yay'", false}, + {"'strict-dynamic'", false}, + {"'sha512-321cba' http://example1.com/foo/", false}, + {"http://example1.com/foo/ 'sha512-321cba'", false}, + {"http://example1.com/foo/ 'nonce-yay'", false}, + {"'none' 'sha512-321cba' http://example1.com/foo/", false}, + {"'none' http://example1.com/foo/ 'sha512-321cba'", false}, + {"'none' http://example1.com/foo/ 'nonce-yay'", false}, + {"'sha512-321cba' 'nonce-yay'", false}, + {"http://example1.com/foo/ 'sha512-321cba' 'nonce-yay'", false}, + {"http://example1.com/foo/ 'sha512-321cba' 'nonce-yay'", false}, + {" 'sha512-321cba' 'nonce-yay' 'strict-dynamic'", false}, + }; + + for (const auto& test : cases) { + SourceListDirective scriptSrc("script-src", test.sources, csp.get()); + EXPECT_EQ(scriptSrc.isNone(), test.expected); + + SourceListDirective styleSrc("form-action", test.sources, csp.get()); + EXPECT_EQ(styleSrc.isNone(), test.expected); + + SourceListDirective imgSrc("frame-src", test.sources, csp.get()); + EXPECT_EQ(styleSrc.isNone(), test.expected); + } +} + } // namespace blink
diff --git a/third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp b/third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp index d1e42eb..2eb31fc9 100644 --- a/third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp +++ b/third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp
@@ -180,6 +180,10 @@ if (!SubframeLoadingDisabler::canLoadFrame(*this)) return; + // We should never have a content frame at the point where we got inserted + // into a tree. + SECURITY_CHECK(!contentFrame()); + setNameAndOpenURL(); }
diff --git a/third_party/libphonenumber/OWNERS b/third_party/libphonenumber/OWNERS index 3660b63..6ff21db 100644 --- a/third_party/libphonenumber/OWNERS +++ b/third_party/libphonenumber/OWNERS
@@ -1,2 +1,2 @@ -isherman@chromium.org +mathp@chromium.org rouslan@chromium.org
diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml index b89fe2e1..199b3e4 100644 --- a/tools/metrics/histograms/histograms.xml +++ b/tools/metrics/histograms/histograms.xml
@@ -63682,6 +63682,17 @@ </summary> </histogram> +<histogram name="Style.LazyUsage.Percent" enum="LazyCSSParseUsage"> + <owner>csharrison@chromium.org</owner> + <summary> + Tracks % of lazy rules that ended up needing to be parsed. A sheet logs + counts into this histogram as it is parsed (i.e. as properties are parsed + lazily). Once a certain percent of rules have been parsed, we log a count + immediately. Note that this implies that a stylesheet which uses all of its + rules will log counts in every bucket. + </summary> +</histogram> + <histogram name="Style.UpdateTime" units="microseconds"> <owner>csharrison@chromium.org</owner> <summary>Microseconds spent in Document::updateStyle.</summary> @@ -91721,6 +91732,16 @@ <int value="268959744" label="NEW_TASK | NEW_DOCUMENT"/> </enum> +<enum name="LazyCSSParseUsage" type="int"> + <int value="0" label=">= 0%"/> + <int value="1" label="> 10%"/> + <int value="2" label="> 25%"/> + <int value="3" label="> 50%"/> + <int value="4" label="> 75%"/> + <int value="5" label="> 90%"/> + <int value="6" label="= 100%"/> +</enum> + <enum name="LevelDBCorruptionRestoreValue" type="int"> <int value="0" label="Database Delete Success"/> <int value="1" label="Database Delete Failure"/>
diff --git a/tools/perf/page_sets/system_health/browsing_stories.py b/tools/perf/page_sets/system_health/browsing_stories.py index f9be0b3..62b843d 100644 --- a/tools/perf/page_sets/system_health/browsing_stories.py +++ b/tools/perf/page_sets/system_health/browsing_stories.py
@@ -40,7 +40,7 @@ self._WaitForNavigation(action_runner) def _NavigateBack(self, action_runner): - action_runner.ExecuteJavaScript('window.history.back()') + action_runner.NavigateBack() self._WaitForNavigation(action_runner)