diff --git a/chrome/android/java/res/layout/accept_languages_item.xml b/chrome/android/java/res/layout/accept_languages_item.xml index d22e680..4012ddfa 100644 --- a/chrome/android/java/res/layout/accept_languages_item.xml +++ b/chrome/android/java/res/layout/accept_languages_item.xml
@@ -8,6 +8,8 @@ xmlns:chrome="http://schemas.android.com/apk/res-auto" android:id="@+id/language_item" android:layout_marginStart="0dp" + android:paddingStart="@dimen/pref_languages_padding" + android:paddingEnd="@dimen/pref_languages_padding" style="@style/ListItemContainer"> <include layout="@layout/modern_list_item_view" />
diff --git a/chrome/android/java/res/layout/accept_languages_list.xml b/chrome/android/java/res/layout/accept_languages_list.xml index cd3e8eef..4d77cbd 100644 --- a/chrome/android/java/res/layout/accept_languages_list.xml +++ b/chrome/android/java/res/layout/accept_languages_list.xml
@@ -3,20 +3,8 @@ Use of this source code is governed by a BSD-style license that can be found in the LICENSE file. --> -<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android" - android:id="@+id/accept_language_list_container" +<android.support.v7.widget.RecyclerView + xmlns:android="http://schemas.android.com/apk/res/android" + android:id="@+id/language_list" android:layout_width="match_parent" - android:layout_height="wrap_content" - android:orientation="vertical" > - - <TextView - android:layout_width="match_parent" - android:layout_height="wrap_content" - android:text="@string/languages_list_prefs_description" /> - - <android.support.v7.widget.RecyclerView - android:id="@+id/language_list" - android:layout_width="match_parent" - android:layout_height="wrap_content" - android:orientation="vertical" /> -</LinearLayout> + android:layout_height="wrap_content" />
diff --git a/chrome/android/java/res/layout/add_languages_main.xml b/chrome/android/java/res/layout/add_languages_main.xml new file mode 100644 index 0000000..4d77cbd --- /dev/null +++ b/chrome/android/java/res/layout/add_languages_main.xml
@@ -0,0 +1,10 @@ +<?xml version="1.0" encoding="utf-8"?> +<!-- Copyright 2017 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. --> + +<android.support.v7.widget.RecyclerView + xmlns:android="http://schemas.android.com/apk/res/android" + android:id="@+id/language_list" + android:layout_width="match_parent" + android:layout_height="wrap_content" />
diff --git a/chrome/android/java/res/layout/languages_preference.xml b/chrome/android/java/res/layout/languages_preference.xml new file mode 100644 index 0000000..cdbc755f --- /dev/null +++ b/chrome/android/java/res/layout/languages_preference.xml
@@ -0,0 +1,38 @@ +<?xml version="1.0" encoding="utf-8"?> +<!-- Copyright 2017 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. --> + +<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android" + android:id="@+id/accept_language_list_container" + style="@style/PreferenceLayout" + android:layout_width="match_parent" + android:layout_height="wrap_content" + android:paddingStart="0dp" + android:paddingEnd="0dp" + android:padding="0dp" + android:orientation="vertical" > + + <TextView + android:layout_width="match_parent" + android:layout_height="wrap_content" + android:padding="@dimen/pref_languages_padding" + android:text="@string/languages_list_prefs_description" /> + + <FrameLayout + android:id="@android:id/widget_frame" + android:layout_width="match_parent" + android:layout_height="wrap_content" /> + + <Button + android:id="@+id/add_language" + android:layout_width="match_parent" + android:layout_height="wrap_content" + android:gravity="center_vertical" + android:padding="@dimen/pref_languages_padding" + android:paddingStart="@dimen/pref_languages_add_button_padding" + android:drawablePadding="@dimen/pref_languages_add_button_padding" + android:text="@string/prefs_add_language" + style="@style/PreferenceLayout" /> + +</LinearLayout>
diff --git a/chrome/android/java/res/menu/languages_action_bar_menu.xml b/chrome/android/java/res/menu/languages_action_bar_menu.xml new file mode 100644 index 0000000..e998be4 --- /dev/null +++ b/chrome/android/java/res/menu/languages_action_bar_menu.xml
@@ -0,0 +1,13 @@ +<?xml version="1.0" encoding="utf-8"?> +<!-- Copyright 2017 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. --> + +<menu xmlns:android="http://schemas.android.com/apk/res/android" + xmlns:chrome="http://schemas.android.com/apk/res-auto" > + <item + android:id="@+id/search" + android:title="@string/search" + chrome:showAsAction="always" + chrome:actionViewClass="android.support.v7.widget.SearchView"/> +</menu>
diff --git a/chrome/android/java/res/values/dimens.xml b/chrome/android/java/res/values/dimens.xml index 540373b..7489e4ef 100644 --- a/chrome/android/java/res/values/dimens.xml +++ b/chrome/android/java/res/values/dimens.xml
@@ -459,6 +459,8 @@ <dimen name="pref_icon_padding">4.6dp</dimen> <dimen name="pref_spinner_padding_end">8dp</dimen> + <dimen name="pref_languages_padding">16dp</dimen> + <dimen name="pref_languages_add_button_padding">24dp</dimen> <dimen name="pref_languages_item_popup_width">260dp</dimen> <!-- Dialog dimensions.
diff --git a/chrome/android/java/res/xml/languages_preferences.xml b/chrome/android/java/res/xml/languages_preferences.xml index 1bd0cf6..64e48b61 100644 --- a/chrome/android/java/res/xml/languages_preferences.xml +++ b/chrome/android/java/res/xml/languages_preferences.xml
@@ -8,14 +8,9 @@ <org.chromium.chrome.browser.preferences.languages.LanguageListPreference android:key="preferred_languages" - android:layout="@layout/custom_preference" + android:layout="@layout/languages_preference" android:widgetLayout="@layout/accept_languages_list" /> - <Preference - android:fragment="org.chromium.chrome.browser.preferences.languages.LanguageSelectionPreferences" - android:key="add_language" - android:title="@string/prefs_add_language" /> - <org.chromium.chrome.browser.preferences.ChromeSwitchPreference android:key="translate_switch" android:summaryOn="@string/languages_offer_translate_switch"
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java b/chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java index 33955b5..ad28ab3 100644 --- a/chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java +++ b/chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java
@@ -13,6 +13,7 @@ import org.chromium.base.VisibleForTesting; import org.chromium.base.annotations.CalledByNative; import org.chromium.chrome.browser.ContentSettingsType; +import org.chromium.chrome.browser.preferences.languages.LanguageItem; import org.chromium.chrome.browser.preferences.website.ContentSetting; import org.chromium.chrome.browser.preferences.website.ContentSettingException; import org.chromium.chrome.browser.search_engines.TemplateUrlService; @@ -187,7 +188,13 @@ } @CalledByNative - private static void copyLanguageList(List<String> list, String[] source) { + private static void addNewLanguageItemToList(List<LanguageItem> list, String code, + String displayName, String nativeDisplayName, boolean supportTranslate) { + list.add(new LanguageItem(code, displayName, nativeDisplayName, supportTranslate)); + } + + @CalledByNative + private static void copyStringArrayToList(List<String> list, String[] source) { list.addAll(Arrays.asList(source)); } @@ -911,14 +918,36 @@ } /** - * Return the list of preferred languages strings. + * @return A sorted list of LanguageItems representing the Chrome accept languages with details. + * Languages that are not supported on Android have been filtered out. */ - public List<String> getChromeLanguageList() { - List<String> list = new ArrayList<>(); - nativeGetChromeLanguageList(list); + public List<LanguageItem> getChromeLanguageList() { + List<LanguageItem> list = new ArrayList<>(); + nativeGetChromeAcceptLanguages(list); return list; } + /** + * @return A sorted list of accept language codes for the current user. + * Note that for the signed-in user, the list might contain some language codes from + * other platforms but not supported on Android. + */ + public List<String> getUserLanguageCodes() { + List<String> list = new ArrayList<>(); + nativeGetUserAcceptLanguages(list); + return list; + } + + /** + * Update accept language for the current user. + * + * @param languageCode A valid language code to update. + * @param add Whether this is an "add" operation or "delete" operation. + */ + public void updateUserAcceptLanguages(String languageCode, boolean add) { + nativeUpdateUserAcceptLanguages(languageCode, add); + } + private native boolean nativeIsContentSettingEnabled(int contentSettingType); private native boolean nativeIsContentSettingManaged(int contentSettingType); private native void nativeSetContentSettingEnabled(int contentSettingType, boolean allow); @@ -1089,5 +1118,7 @@ private native String nativeGetLatestVersionWhenClickedUpdateMenuItem(); private native void nativeSetSupervisedUserId(String supervisedUserId); private native void nativeSetChromeHomePersonalizedOmniboxSuggestionsEnabled(boolean enabled); - private native void nativeGetChromeLanguageList(List<String> list); + private native void nativeGetChromeAcceptLanguages(List<LanguageItem> list); + private native void nativeGetUserAcceptLanguages(List<String> list); + private native void nativeUpdateUserAcceptLanguages(String language, boolean add); }
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/preferences/languages/AddLanguageFragment.java b/chrome/android/java/src/org/chromium/chrome/browser/preferences/languages/AddLanguageFragment.java new file mode 100644 index 0000000..affd76de --- /dev/null +++ b/chrome/android/java/src/org/chromium/chrome/browser/preferences/languages/AddLanguageFragment.java
@@ -0,0 +1,161 @@ +// Copyright 2017 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.preferences.languages; + +import android.app.Activity; +import android.app.Fragment; +import android.content.Intent; +import android.os.Bundle; +import android.support.v7.widget.DividerItemDecoration; +import android.support.v7.widget.LinearLayoutManager; +import android.support.v7.widget.RecyclerView; +import android.support.v7.widget.SearchView; +import android.text.TextUtils; +import android.view.LayoutInflater; +import android.view.Menu; +import android.view.MenuInflater; +import android.view.View; +import android.view.ViewGroup; +import android.view.inputmethod.EditorInfo; + +import org.chromium.chrome.R; + +import java.util.ArrayList; +import java.util.List; +import java.util.Locale; + +/** + * Fragment with a {@link RecyclerView} containing a list of languages that users may add to their + * accept languages. There is a {@link SearchView} on its Actionbar to make a quick lookup. + */ +public class AddLanguageFragment extends Fragment { + static final String INTENT_NEW_ACCEPT_LANGAUGE = "AddLanguageFragment.NewLangauge"; + + /** + * A host to launch AddLanguageFragment and receive the result. + */ + interface Launcher { + /** + * Launches AddLanguageFragment. + */ + void launchAddLanguage(); + } + + private class LanguageSearchListAdapter extends LanguageListBaseAdapter { + @Override + public void onBindViewHolder(LanguageRowViewHolder holder, int position) { + holder.bindView(getItemByPosition(position), mItemClickListener, null, 0); + } + + /** + * Called to perform a search. + * @param query The text to search for. + */ + private void search(String query) { + if (TextUtils.isEmpty(query)) { + reload(mFullLanguageList); + return; + } + + Locale locale = Locale.getDefault(); + query = query.trim().toLowerCase(locale); + List<LanguageItem> results = new ArrayList<>(); + for (LanguageItem item : mFullLanguageList) { + // TODO(crbug/783049): Consider searching in item's native display name and + // language code too. + if (item.getDisplayName().toLowerCase(locale).contains(query)) { + results.add(item); + } + } + reload(results); + } + } + + // The view for searching the list of items. + private SearchView mSearchView; + + // If not blank, represents a substring to use to search for language names. + private String mSearch; + + private RecyclerView mRecyclerView; + private LanguageSearchListAdapter mAdapter; + private List<LanguageItem> mFullLanguageList; + private LanguageListBaseAdapter.ItemClickListener mItemClickListener; + + @Override + public void onCreate(Bundle savedInstanceState) { + super.onCreate(savedInstanceState); + getActivity().setTitle(R.string.prefs_add_language); + setHasOptionsMenu(true); + // TODO(crbug/783049): Record impression. + } + + @Override + public View onCreateView( + LayoutInflater inflater, ViewGroup container, Bundle savedInstanceState) { + // Inflate the layout for this fragment. + View view = inflater.inflate(R.layout.add_languages_main, container, false); + mSearch = ""; + final Activity activity = getActivity(); + + mRecyclerView = (RecyclerView) view.findViewById(R.id.language_list); + LinearLayoutManager layoutMangager = new LinearLayoutManager(activity); + mRecyclerView.setLayoutManager(layoutMangager); + mRecyclerView.addItemDecoration( + new DividerItemDecoration(activity, layoutMangager.getOrientation())); + + mFullLanguageList = LanguagesManager.getInstance().getLanguageItemsExcludingUserAccept(); + mItemClickListener = new LanguageListBaseAdapter.ItemClickListener() { + @Override + public void onLanguageClicked(LanguageItem item) { + Intent intent = new Intent(); + intent.putExtra(INTENT_NEW_ACCEPT_LANGAUGE, item.getCode()); + activity.setResult(activity.RESULT_OK, intent); + activity.finish(); + } + }; + mAdapter = new LanguageSearchListAdapter(); + + mRecyclerView.setAdapter(mAdapter); + mAdapter.reload(mFullLanguageList); + return view; + } + + @Override + public void onCreateOptionsMenu(Menu menu, MenuInflater inflater) { + menu.clear(); + inflater.inflate(R.menu.languages_action_bar_menu, menu); + + mSearchView = (SearchView) menu.findItem(R.id.search).getActionView(); + mSearchView.setImeOptions(EditorInfo.IME_FLAG_NO_FULLSCREEN); + + mSearchView.setOnCloseListener(new SearchView.OnCloseListener() { + @Override + public boolean onClose() { + mSearch = ""; + mAdapter.reload(mFullLanguageList); + return false; + } + }); + + mSearchView.setOnQueryTextListener(new SearchView.OnQueryTextListener() { + @Override + public boolean onQueryTextSubmit(String query) { + return true; + } + + @Override + public boolean onQueryTextChange(String query) { + if (TextUtils.isEmpty(query) || TextUtils.equals(query, mSearch)) { + return true; + } + + mSearch = query; + mAdapter.search(mSearch); + return true; + } + }); + } +}
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/preferences/languages/LanguageItem.java b/chrome/android/java/src/org/chromium/chrome/browser/preferences/languages/LanguageItem.java new file mode 100644 index 0000000..252da20 --- /dev/null +++ b/chrome/android/java/src/org/chromium/chrome/browser/preferences/languages/LanguageItem.java
@@ -0,0 +1,61 @@ +// Copyright 2017 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.preferences.languages; + +/** + * Simple object representing the language item. + */ +public class LanguageItem { + private final String mCode; + + private final String mDisplayName; + + private final String mNativeDisplayName; + + private final boolean mSupportTranslate; + + /** + * Creates a new {@link LanguageItem}. + * @param code The string resource id for the text to show for this item. + * @param displayName The display name of the language in the current app locale. + * @param nativeDisplayName The display name of the language in the language's locale. + * @param supportTranslate Whether Chrome supports translate for this language. + */ + public LanguageItem( + String code, String displayName, String nativeDisplayName, boolean supportTranslate) { + mCode = code; + mDisplayName = displayName; + mNativeDisplayName = nativeDisplayName; + mSupportTranslate = supportTranslate; + } + + /** + * @return The ISO code of the language item. + */ + public String getCode() { + return mCode; + } + + /** + * @return The display name of the language in the current app locale. + */ + public String getDisplayName() { + return mDisplayName; + } + + /** + * @return The display name of the language in the language's locale. + */ + public String getNativeDisplayName() { + return mNativeDisplayName; + } + + /** + * @return Whether Chrome supports translate for this language. + */ + public boolean isSupported() { + return mSupportTranslate; + } +}
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/preferences/languages/LanguageListBaseAdapter.java b/chrome/android/java/src/org/chromium/chrome/browser/preferences/languages/LanguageListBaseAdapter.java new file mode 100644 index 0000000..0ce7c77e --- /dev/null +++ b/chrome/android/java/src/org/chromium/chrome/browser/preferences/languages/LanguageListBaseAdapter.java
@@ -0,0 +1,130 @@ +// Copyright 2017 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.preferences.languages; + +import android.support.annotation.DrawableRes; +import android.support.v7.widget.RecyclerView; +import android.text.TextUtils; +import android.view.LayoutInflater; +import android.view.View; +import android.view.ViewGroup; +import android.widget.TextView; + +import org.chromium.chrome.R; +import org.chromium.chrome.browser.widget.ListMenuButton; +import org.chromium.chrome.browser.widget.TintedImageView; + +import java.util.ArrayList; +import java.util.List; + +/** + * BaseAdapter for {@link RecyclerView}. It manages languages to list there. + */ +public class LanguageListBaseAdapter + extends RecyclerView.Adapter<LanguageListBaseAdapter.LanguageRowViewHolder> { + /** + * Listener used to respond to click event on a langauge item. + */ + interface ItemClickListener { + /** + * @param item The clicked LanguageItem. + */ + void onLanguageClicked(LanguageItem item); + } + + static class LanguageRowViewHolder extends RecyclerView.ViewHolder { + private View mRow; + private TextView mTitle; + private TextView mDescription; + + private TintedImageView mStartIcon; + private ListMenuButton mMoreButton; + + private ItemClickListener mItemClickListener; + + LanguageRowViewHolder(View view) { + super(view); + mRow = view; + mTitle = (TextView) view.findViewById(R.id.title); + mDescription = (TextView) view.findViewById(R.id.description); + + mStartIcon = (TintedImageView) view.findViewById(R.id.icon_view); + mMoreButton = (ListMenuButton) view.findViewById(R.id.more); + } + + private void setDisplayName(LanguageItem item) { + mTitle.setText(item.getDisplayName()); + // Avoid duplicate display names. + if (TextUtils.equals(item.getDisplayName(), item.getNativeDisplayName())) { + mDescription.setVisibility(View.GONE); + } else { + mDescription.setVisibility(View.VISIBLE); + mDescription.setText(item.getNativeDisplayName()); + } + } + + /** + * Binds this {@link LanguageRowViewHolder} with the given params. + * @param item The {@link LanguageItem} with language details. + * @param itemClickListener Callback for the click event on this row. + * @param menuButtonDelegate A {@link ListMenuButton.Delegate} to set up an optional menu + * button at the end of this row. + * @param startIconId An optional icon at the beginning of this row. + */ + void bindView(LanguageItem item, ItemClickListener itemClickListener, + ListMenuButton.Delegate menuButtonDelegate, @DrawableRes int startIconId) { + setDisplayName(item); + + if (startIconId != 0) { + mStartIcon.setImageResource(startIconId); + } else { + mStartIcon.setVisibility(View.GONE); + } + + if (menuButtonDelegate != null) { + mMoreButton.setDelegate(menuButtonDelegate); + } else { + mMoreButton.setVisibility(View.GONE); + } + + if (itemClickListener != null) { + mRow.setOnClickListener(view -> itemClickListener.onLanguageClicked(item)); + } + } + } + + private List<LanguageItem> mLanguageList; + + LanguageListBaseAdapter() { + mLanguageList = new ArrayList<>(); + } + + @Override + public LanguageRowViewHolder onCreateViewHolder(ViewGroup parent, int viewType) { + View row = LayoutInflater.from(parent.getContext()) + .inflate(R.layout.accept_languages_item, parent, false); + return new LanguageRowViewHolder(row); + } + + @Override + public void onBindViewHolder(LanguageRowViewHolder holder, int position) { + holder.bindView(mLanguageList.get(position), null, null, 0); + } + + @Override + public int getItemCount() { + return mLanguageList.size(); + } + + LanguageItem getItemByPosition(int position) { + return mLanguageList.get(position); + } + + void reload(List<LanguageItem> languageList) { + mLanguageList.clear(); + mLanguageList.addAll(languageList); + notifyDataSetChanged(); + } +}
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/preferences/languages/LanguageListPreference.java b/chrome/android/java/src/org/chromium/chrome/browser/preferences/languages/LanguageListPreference.java index e6c4bde..32b1abb8 100644 --- a/chrome/android/java/src/org/chromium/chrome/browser/preferences/languages/LanguageListPreference.java +++ b/chrome/android/java/src/org/chromium/chrome/browser/preferences/languages/LanguageListPreference.java
@@ -6,121 +6,119 @@ import android.content.Context; import android.preference.Preference; +import android.support.v7.widget.DividerItemDecoration; import android.support.v7.widget.LinearLayoutManager; import android.support.v7.widget.RecyclerView; import android.util.AttributeSet; -import android.view.LayoutInflater; import android.view.View; import android.view.ViewGroup; -import android.widget.TextView; +import android.widget.Button; +import org.chromium.base.ApiCompatibilityUtils; import org.chromium.chrome.R; -import org.chromium.chrome.browser.preferences.PrefServiceBridge; import org.chromium.chrome.browser.widget.ListMenuButton; import org.chromium.chrome.browser.widget.ListMenuButton.Item; -import org.chromium.chrome.browser.widget.TintedImageView; - -import java.util.List; +import org.chromium.chrome.browser.widget.TintedDrawable; /** * A preference that displays the current accept language list. */ public class LanguageListPreference extends Preference { + // TODO(crbug/783049): Make the item in the list drag-able. + private static class LanguageListAdapter + extends LanguageListBaseAdapter implements LanguagesManager.AcceptLanguageObserver { + private Context mContext; + + LanguageListAdapter(Context context) { + super(); + mContext = context; + } + + @Override + public void onBindViewHolder( + LanguageListBaseAdapter.LanguageRowViewHolder holder, int position) { + final LanguageItem info = getItemByPosition(position); + holder.bindView(info, null, new ListMenuButton.Delegate() { + @Override + public Item[] getItems() { + return new Item[] { + new Item(mContext, R.string.languages_item_option_offer_to_translate, + R.drawable.ic_check_googblue_24dp, info.isSupported()), + new Item(mContext, R.string.remove, getItemCount() > 1)}; + } + + @Override + public void onItemSelected(Item item) { + if (item.getTextId() == R.string.languages_item_option_offer_to_translate) { + // TODO(crbug/783049): Handle "offer to translate" event. + } else if (item.getTextId() == R.string.remove) { + LanguagesManager.getInstance().removeFromAcceptLanguages(info.getCode()); + } + } + }, R.drawable.ic_drag_handle_grey600_24dp); + } + + @Override + public void onDataUpdated() { + reload(LanguagesManager.getInstance().getUserAcceptLanguageItems()); + } + } + + private View mView; + private Button mAddLanguageButton; + private RecyclerView mRecyclerView; + private LanguageListAdapter mAdapter; + + private AddLanguageFragment.Launcher mLauncher; + public LanguageListPreference(Context context, AttributeSet attrs) { super(context, attrs); + mAdapter = new LanguageListAdapter(context); + } + + @Override + public View onCreateView(ViewGroup parent) { + assert mLauncher != null; + + if (mView != null) return mView; + + mView = super.onCreateView(parent); + + mAddLanguageButton = (Button) mView.findViewById(R.id.add_language); + ApiCompatibilityUtils.setCompoundDrawablesRelativeWithIntrinsicBounds(mAddLanguageButton, + TintedDrawable.constructTintedDrawable( + getContext().getResources(), R.drawable.plus, R.color.pref_accent_color), + null, null, null); + mAddLanguageButton.setOnClickListener(view -> mLauncher.launchAddLanguage()); + + mRecyclerView = (RecyclerView) mView.findViewById(R.id.language_list); + LinearLayoutManager layoutMangager = new LinearLayoutManager(getContext()); + mRecyclerView.setLayoutManager(layoutMangager); + mRecyclerView.addItemDecoration( + new DividerItemDecoration(getContext(), layoutMangager.getOrientation())); + + return mView; } @Override protected void onBindView(View view) { super.onBindView(view); - RecyclerView listView = (RecyclerView) view.findViewById(R.id.language_list); - listView.setLayoutManager(new LinearLayoutManager(getContext())); - - // Get accepted language list from native. - List<String> languageList = PrefServiceBridge.getInstance().getChromeLanguageList(); - - listView.setAdapter(new LanguageListAdapter(languageList)); - } - - // TODO(crbug/783049): Pull all the inner classes below out and make the item in the list - // drag-able. - private static class LanguageListAdapter extends RecyclerView.Adapter<LanguageItemViewHolder> { - private List<String> mLanguageList; - - LanguageListAdapter(List<String> languageList) { - mLanguageList = languageList; - } - - @Override - public LanguageItemViewHolder onCreateViewHolder(ViewGroup parent, int viewType) { - View languageItem = LayoutInflater.from(parent.getContext()) - .inflate(R.layout.accept_languages_item, parent, false); - return new LanguageItemViewHolder(languageItem); - } - - @Override - public void onBindViewHolder(LanguageItemViewHolder holder, int position) { - String code = mLanguageList.get(position); - // TODO(crbug/783049): Get language name by code and Locale from LocalizationUtils. - // displayLocale = context.getResources().getConfiguration().locale; - holder.setupUI(code); - } - - @Override - public int getItemCount() { - return mLanguageList.size(); + // We do not want the RecyclerView to be announced by screen readers every time + // the view is bound. + if (mRecyclerView.getAdapter() != mAdapter) { + mRecyclerView.setAdapter(mAdapter); + LanguagesManager.getInstance().setAcceptLanguageObserver(mAdapter); + // Initialize accept language list. + mAdapter.reload(LanguagesManager.getInstance().getUserAcceptLanguageItems()); } } - private static class LanguageItemViewHolder - extends RecyclerView.ViewHolder implements ListMenuButton.Delegate { - private static final int OFFER_TRANSLATE_POSITION = 0; - - // Menu items definition. - private final Item[] mItems; - - private Context mContext; - private View mRow; - private TextView mTitle; - private TintedImageView mStartIcon; - private ListMenuButton mMoreButton; - - LanguageItemViewHolder(View view) { - super(view); - mContext = view.getContext(); - mRow = view; - mTitle = (TextView) view.findViewById(R.id.title); - mStartIcon = (TintedImageView) view.findViewById(R.id.icon_view); - mMoreButton = (ListMenuButton) view.findViewById(R.id.more); - - // TODO(crbug/783049): Set "enabled" based on whether Chrome supports to translate this - // language. - mItems = new Item[] { - new Item(mContext, R.string.languages_item_option_offer_to_translate, - R.drawable.ic_check_googblue_24dp, true), - new Item(mContext, R.string.remove, true)}; - } - - private void setupUI(String title) { - mTitle.setText(title); - mStartIcon.setImageResource(R.drawable.ic_drag_handle_grey600_24dp); - mMoreButton.setDelegate(this); - } - - // ListMenuButton.Delegate implementation. - @Override - public Item[] getItems() { - return mItems; - } - - @Override - public void onItemSelected(Item item) { - if (item.getTextId() == R.string.languages_item_option_offer_to_translate) { - // TODO(crbug/783049): Handle "offer to translate" event. - } else if (item.getTextId() == R.string.remove) { - // TODO(crbug/783049): Handle "remove" event. - } - } + /** + * Register a launcher for AddLanguageFragment. Preference's host fragment should call + * this in its onCreate(). + */ + void registerActivityLauncher(AddLanguageFragment.Launcher launcher) { + mLauncher = launcher; } }
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/preferences/languages/LanguagesManager.java b/chrome/android/java/src/org/chromium/chrome/browser/preferences/languages/LanguagesManager.java new file mode 100644 index 0000000..494f430 --- /dev/null +++ b/chrome/android/java/src/org/chromium/chrome/browser/preferences/languages/LanguagesManager.java
@@ -0,0 +1,123 @@ +// Copyright 2017 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.preferences.languages; + +import org.chromium.chrome.browser.preferences.PrefServiceBridge; + +import java.util.ArrayList; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; + +/** + * Manages languages details for languages settings. + * + * The LanguagesManager is responsible for fetching languages details from native. + */ +class LanguagesManager { + /** + * An observer interface that allows other classes to know when the accept language list is + * updated in native side. + */ + interface AcceptLanguageObserver { + /** + * Called when the accept languages for the current user are updated. + */ + void onDataUpdated(); + } + + private static LanguagesManager sManager; + + private final PrefServiceBridge mPrefServiceBridge; + private final Map<String, LanguageItem> mLanguagesMap; + + private AcceptLanguageObserver mObserver; + + private LanguagesManager() { + mPrefServiceBridge = PrefServiceBridge.getInstance(); + + // Get all language data from native. + mLanguagesMap = new LinkedHashMap<>(); + for (LanguageItem item : mPrefServiceBridge.getChromeLanguageList()) { + mLanguagesMap.put(item.getCode(), item); + } + } + + private void notifyAcceptLanguageObserver() { + if (mObserver != null) mObserver.onDataUpdated(); + } + + /** + * Sets the observer tracking the user accept languages changes. + */ + public void setAcceptLanguageObserver(AcceptLanguageObserver observer) { + mObserver = observer; + } + + /** + * @return A list of LanguageItems for the current user's accept languages. + */ + public List<LanguageItem> getUserAcceptLanguageItems() { + // Always read the latest user accept language code list from native. + List<String> codes = mPrefServiceBridge.getUserLanguageCodes(); + + List<LanguageItem> results = new ArrayList<>(); + // Keep the same order as accept language codes list. + for (String code : codes) { + // Check language code and only languages supported on Android are added in. + if (mLanguagesMap.containsKey(code)) results.add(mLanguagesMap.get(code)); + } + return results; + } + + /** + * @return A list of LanguageItems, excluding the current user's accept languages. + */ + public List<LanguageItem> getLanguageItemsExcludingUserAccept() { + // Always read the latest user accept language code list from native. + List<String> codes = mPrefServiceBridge.getUserLanguageCodes(); + + List<LanguageItem> results = new ArrayList<>(); + // Keep the same order as mLanguagesMap. + for (LanguageItem item : mLanguagesMap.values()) { + if (!codes.contains(item.getCode())) results.add(item); + } + return results; + } + + /** + * Add a language to the current user's accept languages. + * @param code The language code to remove. + */ + public void addToAcceptLanguages(String code) { + mPrefServiceBridge.updateUserAcceptLanguages(code, true /* is_add */); + notifyAcceptLanguageObserver(); + } + + /** + * Remove a language from the current user's accept languages. + * @param code The language code to remove. + */ + public void removeFromAcceptLanguages(String code) { + mPrefServiceBridge.updateUserAcceptLanguages(code, false /* is_add */); + notifyAcceptLanguageObserver(); + } + + /** + * Get the static instance of ChromePreferenceManager if it exists else create it. + * @return the LanguagesManager singleton. + */ + public static LanguagesManager getInstance() { + if (sManager == null) sManager = new LanguagesManager(); + return sManager; + } + + /** + * Called to release unused resources. + */ + public static void recycle() { + sManager = null; + } +}
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/preferences/languages/LanguagesPreferences.java b/chrome/android/java/src/org/chromium/chrome/browser/preferences/languages/LanguagesPreferences.java index eb7e639e..e614e98 100644 --- a/chrome/android/java/src/org/chromium/chrome/browser/preferences/languages/LanguagesPreferences.java +++ b/chrome/android/java/src/org/chromium/chrome/browser/preferences/languages/LanguagesPreferences.java
@@ -4,6 +4,7 @@ package org.chromium.chrome.browser.preferences.languages; +import android.content.Intent; import android.os.Bundle; import android.preference.Preference; import android.preference.Preference.OnPreferenceChangeListener; @@ -14,20 +15,21 @@ import org.chromium.chrome.browser.preferences.ManagedPreferenceDelegate; import org.chromium.chrome.browser.preferences.PrefServiceBridge; import org.chromium.chrome.browser.preferences.PreferenceUtils; -import org.chromium.chrome.browser.widget.TintedDrawable; +import org.chromium.chrome.browser.preferences.PreferencesLauncher; /** * Settings fragment that displays information about Chrome languages, which allow users to * seamlessly find and manage their languages preferences across platforms. */ -public class LanguagesPreferences extends PreferenceFragment { - // The keys for each prefernce shown on the langauges page. - static final String ADD_LANGUAGE_KEY = "add_language"; +public class LanguagesPreferences + extends PreferenceFragment implements AddLanguageFragment.Launcher { + private static final int REQUEST_CODE_ADD_LANGUAGES = 1; + + // The keys for each preference shown on the languages page. static final String PREFERRED_LANGUAGES_KEY = "preferred_languages"; static final String TRANSLATE_SWITCH_KEY = "translate_switch"; private LanguageListPreference mLanguageListPref; - private Preference mAddLanguagePref; @Override public void onCreate(Bundle savedInstanceState) { @@ -36,11 +38,8 @@ getActivity().setTitle(R.string.prefs_languages); PreferenceUtils.addPreferencesFromResource(this, R.xml.languages_preferences); - mAddLanguagePref = findPreference(ADD_LANGUAGE_KEY); - mAddLanguagePref.setIcon(TintedDrawable.constructTintedDrawable( - getResources(), R.drawable.plus, R.color.pref_accent_color)); - mLanguageListPref = (LanguageListPreference) findPreference(PREFERRED_LANGUAGES_KEY); + mLanguageListPref.registerActivityLauncher(this); ChromeSwitchPreference translateSwitch = (ChromeSwitchPreference) findPreference(TRANSLATE_SWITCH_KEY); @@ -61,4 +60,27 @@ } }); } + + @Override + public void onDetach() { + super.onDetach(); + LanguagesManager.recycle(); + } + + @Override + public void onActivityResult(int requestCode, int resultCode, Intent data) { + super.onActivityResult(requestCode, resultCode, data); + if (requestCode == REQUEST_CODE_ADD_LANGUAGES && resultCode == getActivity().RESULT_OK) { + String code = data.getStringExtra(AddLanguageFragment.INTENT_NEW_ACCEPT_LANGAUGE); + LanguagesManager.getInstance().addToAcceptLanguages(code); + } + } + + @Override + public void launchAddLanguage() { + // Launch preference activity with AddLanguageFragment. + Intent intent = PreferencesLauncher.createIntentForSettingsPage( + getActivity(), AddLanguageFragment.class.getName()); + startActivityForResult(intent, REQUEST_CODE_ADD_LANGUAGES); + } }
diff --git a/chrome/android/java_sources.gni b/chrome/android/java_sources.gni index fccb374c..4acba6e 100644 --- a/chrome/android/java_sources.gni +++ b/chrome/android/java_sources.gni
@@ -959,9 +959,12 @@ "java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionProxyUma.java", "java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionStatsPreference.java", "java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionDataUseItem.java", + "java/src/org/chromium/chrome/browser/preferences/languages/AddLanguageFragment.java", + "java/src/org/chromium/chrome/browser/preferences/languages/LanguagesManager.java", "java/src/org/chromium/chrome/browser/preferences/languages/LanguagesPreferences.java", + "java/src/org/chromium/chrome/browser/preferences/languages/LanguageItem.java", + "java/src/org/chromium/chrome/browser/preferences/languages/LanguageListBaseAdapter.java", "java/src/org/chromium/chrome/browser/preferences/languages/LanguageListPreference.java", - "java/src/org/chromium/chrome/browser/preferences/languages/LanguageSelectionPreferences.java", "java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java", "java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java", "java/src/org/chromium/chrome/browser/preferences/privacy/BandwidthType.java",
diff --git a/chrome/browser/android/preferences/pref_service_bridge.cc b/chrome/browser/android/preferences/pref_service_bridge.cc index b0bd719..998111e 100644 --- a/chrome/browser/android/preferences/pref_service_bridge.cc +++ b/chrome/browser/android/preferences/pref_service_bridge.cc
@@ -1145,7 +1145,28 @@ is_enabled); } -static void JNI_PrefServiceBridge_GetChromeLanguageList( +static void JNI_PrefServiceBridge_GetChromeAcceptLanguages( + JNIEnv* env, + const JavaParamRef<jobject>& obj, + const JavaParamRef<jobject>& list) { + std::unique_ptr<translate::TranslatePrefs> translate_prefs = + ChromeTranslateClient::CreateTranslatePrefs(GetPrefService()); + + std::vector<translate::TranslateLanguageInfo> languages; + translate_prefs->GetLanguageInfoList( + g_browser_process->GetApplicationLocale(), + translate_prefs->IsTranslateAllowedByPolicy(), &languages); + + for (const auto& info : languages) { + Java_PrefServiceBridge_addNewLanguageItemToList( + env, list, ConvertUTF8ToJavaString(env, info.code), + ConvertUTF8ToJavaString(env, info.display_name), + ConvertUTF8ToJavaString(env, info.native_display_name), + info.supports_translate); + } +} + +static void JNI_PrefServiceBridge_GetUserAcceptLanguages( JNIEnv* env, const JavaParamRef<jobject>& obj, const JavaParamRef<jobject>& list) { @@ -1154,8 +1175,24 @@ std::vector<std::string> languages; translate_prefs->GetLanguageList(&languages); - Java_PrefServiceBridge_copyLanguageList(env, list, - ToJavaArrayOfStrings(env, languages)); + Java_PrefServiceBridge_copyStringArrayToList( + env, list, ToJavaArrayOfStrings(env, languages)); +} + +static void JNI_PrefServiceBridge_UpdateUserAcceptLanguages( + JNIEnv* env, + const JavaParamRef<jobject>& obj, + const JavaParamRef<jstring>& language, + jboolean is_add) { + std::unique_ptr<translate::TranslatePrefs> translate_prefs = + ChromeTranslateClient::CreateTranslatePrefs(GetPrefService()); + std::string language_code(ConvertJavaStringToUTF8(env, language)); + + if (is_add) { + translate_prefs->AddToLanguageList(language_code, false /*force_blocked=*/); + } else { + translate_prefs->RemoveFromLanguageList(language_code); + } } const char* PrefServiceBridge::GetPrefNameExposedToJava(int pref_index) {
diff --git a/chrome/browser/android/search_permissions/search_permissions_service.cc b/chrome/browser/android/search_permissions/search_permissions_service.cc index f46aef8338..7d7c12c7 100644 --- a/chrome/browser/android/search_permissions/search_permissions_service.cc +++ b/chrome/browser/android/search_permissions/search_permissions_service.cc
@@ -179,6 +179,10 @@ void SearchPermissionsService::OnDSEChanged() { InitializeSettingsIfNeeded(); + // If we didn't initialize properly because there is no DSE don't do anything. + if (!pref_service_->HasPrefPath(prefs::kDSEPermissionsSettings)) + return; + PrefValue pref = GetDSEPref(); base::string16 new_dse_name = delegate_->GetDSEName(); @@ -187,24 +191,19 @@ GURL old_dse_origin(pref.dse_origin); GURL new_dse_origin = delegate_->GetDSEOrigin().GetURL(); - // This can happen in tests. - // TODO(raymes): It turns out this can also happen if the DSE is disabled by - // policy. This is another case we need to correctly handle. - if (!new_dse_origin.is_valid()) - return; - - // Don't do anything if the DSE name/origin hasn't changed. + // Don't do anything if the DSE origin hasn't changed. if (old_dse_origin == new_dse_origin) return; - ContentSetting geolocation_setting_to_restore = UpdatePermission( - CONTENT_SETTINGS_TYPE_GEOLOCATION, old_dse_origin, new_dse_origin, - pref.geolocation_setting_to_restore, old_dse_name != new_dse_name); + ContentSetting geolocation_setting_to_restore = + UpdatePermissionAndReturnPrevious( + CONTENT_SETTINGS_TYPE_GEOLOCATION, old_dse_origin, new_dse_origin, + pref.geolocation_setting_to_restore, old_dse_name != new_dse_name); ContentSetting notifications_setting_to_restore = pref.notifications_setting_to_restore; // Only update the notifications part of the pref if the feature is enabled. if (base::FeatureList::IsEnabled(features::kGrantNotificationsToDSE)) { - notifications_setting_to_restore = UpdatePermission( + notifications_setting_to_restore = UpdatePermissionAndReturnPrevious( CONTENT_SETTINGS_TYPE_NOTIFICATIONS, old_dse_origin, new_dse_origin, pref.notifications_setting_to_restore, old_dse_name != new_dse_name); } @@ -217,19 +216,13 @@ SetDSEPref(pref); } -ContentSetting SearchPermissionsService::UpdatePermission( +ContentSetting SearchPermissionsService::RestoreOldSettingAndReturnPrevious( + const GURL& dse_origin, ContentSettingsType type, - const GURL& old_dse_origin, - const GURL& new_dse_origin, - ContentSetting old_dse_setting_to_restore, - bool dse_name_changed) { - // Remove any embargo on the URL. - PermissionDecisionAutoBlocker::GetForProfile(profile_)->RemoveEmbargoByUrl( - new_dse_origin, type); - + ContentSetting setting_to_restore) { // Read the current value of the old DSE. This is the DSE setting that we want // to try to apply to the new DSE origin. - ContentSetting dse_setting = GetContentSetting(old_dse_origin, type); + ContentSetting dse_setting = GetContentSetting(dse_origin, type); // The user's setting should never be ASK while an origin is the DSE. There // should be no way for the user to reset the DSE content setting to ASK. @@ -243,9 +236,25 @@ // Restore the setting for the old origin. If the user has changed the setting // since the origin became the DSE, we reset the setting so the user will be // prompted. - if (old_dse_setting_to_restore != dse_setting) - old_dse_setting_to_restore = CONTENT_SETTING_ASK; - SetContentSetting(old_dse_origin, type, old_dse_setting_to_restore); + if (setting_to_restore != dse_setting) + setting_to_restore = CONTENT_SETTING_ASK; + SetContentSetting(dse_origin, type, setting_to_restore); + + return dse_setting; +} + +ContentSetting SearchPermissionsService::UpdatePermissionAndReturnPrevious( + ContentSettingsType type, + const GURL& old_dse_origin, + const GURL& new_dse_origin, + ContentSetting old_dse_setting_to_restore, + bool dse_name_changed) { + // Remove any embargo on the URL. + PermissionDecisionAutoBlocker::GetForProfile(profile_)->RemoveEmbargoByUrl( + new_dse_origin, type); + + ContentSetting dse_setting = RestoreOldSettingAndReturnPrevious( + old_dse_origin, type, old_dse_setting_to_restore); ContentSetting new_dse_setting_to_restore = GetContentSetting(new_dse_origin, type); @@ -276,10 +285,28 @@ void SearchPermissionsService::InitializeSettingsIfNeeded() { GURL dse_origin = delegate_->GetDSEOrigin().GetURL(); - // This can happen in tests or if the DSE is disabled by policy. We defer - // initialization until later. - if (!dse_origin.is_valid()) + // This can happen in tests or if the DSE is disabled by policy. If that's + // the case, we restore the old settings and erase the pref. This means that + // things will be re-initialized to defaults when the DSE is no longer under + // enterprise policy. + if (!dse_origin.is_valid()) { + if (pref_service_->HasPrefPath(prefs::kDSEPermissionsSettings)) { + PrefValue pref = GetDSEPref(); + GURL old_dse_origin(pref.dse_origin); + RestoreOldSettingAndReturnPrevious(old_dse_origin, + CONTENT_SETTINGS_TYPE_GEOLOCATION, + pref.geolocation_setting_to_restore); + if (pref.notifications_setting_to_restore != CONTENT_SETTING_DEFAULT) { + RestoreOldSettingAndReturnPrevious( + old_dse_origin, CONTENT_SETTINGS_TYPE_NOTIFICATIONS, + pref.notifications_setting_to_restore); + } + pref_service_->ClearPref(prefs::kDSEPermissionsSettings); + } + + // Defer initialization until a search engine becomes the DSE. return; + } // Initialize the pref for geolocation if it hasn't been initialized yet. if (!pref_service_->HasPrefPath(prefs::kDSEPermissionsSettings)) { @@ -323,7 +350,7 @@ PrefValue pref; pref.dse_name = delegate_->GetDSEName(); - pref.dse_origin = delegate_->GetDSEOrigin().GetURL().spec(); + pref.dse_origin = dse_origin.spec(); pref.geolocation_setting_to_restore = geolocation_setting_to_restore; pref.notifications_setting_to_restore = CONTENT_SETTING_DEFAULT; SetDSEPref(pref); @@ -353,8 +380,9 @@ pref.notifications_setting_to_restore != CONTENT_SETTING_DEFAULT) { // Handle the case where the feature has been disabled. Restore the pref // value and reset the setting to restore to DEFAULT. - SetContentSetting(dse_origin, CONTENT_SETTINGS_TYPE_NOTIFICATIONS, - pref.notifications_setting_to_restore); + RestoreOldSettingAndReturnPrevious(GURL(pref.dse_origin), + CONTENT_SETTINGS_TYPE_NOTIFICATIONS, + pref.notifications_setting_to_restore); pref.notifications_setting_to_restore = CONTENT_SETTING_DEFAULT; SetDSEPref(pref); }
diff --git a/chrome/browser/android/search_permissions/search_permissions_service.h b/chrome/browser/android/search_permissions/search_permissions_service.h index 9cdd689..3db96f8 100644 --- a/chrome/browser/android/search_permissions/search_permissions_service.h +++ b/chrome/browser/android/search_permissions/search_permissions_service.h
@@ -106,11 +106,21 @@ // geolocation disclosure so that it will be shown again. void OnDSEChanged(); - ContentSetting UpdatePermission(ContentSettingsType type, - const GURL& old_dse_origin, - const GURL& new_dse_origin, - ContentSetting old_setting, - bool dse_name_changed); + // Restore the setting for an origin before it became the DSE. Returns the + // setting that the origin was set to before restoring the old value. + ContentSetting RestoreOldSettingAndReturnPrevious( + const GURL& dse_origin, + ContentSettingsType type, + ContentSetting setting_to_restore); + + // Helper function for OnDSEChanged which transitions the DSE setting for a + // specific permission. It returns the content setting to be restored later + // for |new_dse_origin|. + ContentSetting UpdatePermissionAndReturnPrevious(ContentSettingsType type, + const GURL& old_dse_origin, + const GURL& new_dse_origin, + ContentSetting old_setting, + bool dse_name_changed); // Initialize the DSE permission settings if they haven't already been // initialized. Also, if they haven't been initialized, reset whether the DSE
diff --git a/chrome/browser/android/search_permissions/search_permissions_service_unittest.cc b/chrome/browser/android/search_permissions/search_permissions_service_unittest.cc index 47baa137..d9b5937 100644 --- a/chrome/browser/android/search_permissions/search_permissions_service_unittest.cc +++ b/chrome/browser/android/search_permissions/search_permissions_service_unittest.cc
@@ -58,11 +58,15 @@ dse_changed_callback_ = callback; } - void SetDSEOrigin(const std::string& dse_origin) { - dse_origin_ = url::Origin::Create(GURL(dse_origin)); + void ChangeDSEOrigin(const std::string& dse_origin) { + set_dse_origin(dse_origin); dse_changed_callback_.Run(); } + void set_dse_origin(const std::string& dse_origin) { + dse_origin_ = url::Origin::Create(GURL(dse_origin)); + } + private: url::Origin dse_origin_; base::Closure dse_changed_callback_; @@ -176,34 +180,34 @@ for (ContentSettingsType type : {CONTENT_SETTINGS_TYPE_GEOLOCATION, CONTENT_SETTINGS_TYPE_NOTIFICATIONS}) { // DSE setting initialized to true if the content setting is ALLOW. - test_delegate()->SetDSEOrigin(kGoogleURL); + test_delegate()->ChangeDSEOrigin(kGoogleURL); SetContentSetting(kGoogleURL, type, CONTENT_SETTING_ALLOW); ReinitializeService(true /* clear_pref */); EXPECT_EQ(CONTENT_SETTING_ALLOW, GetContentSetting(kGoogleURL, type)); // Check that the correct value is restored when changing the DSE. - test_delegate()->SetDSEOrigin(kExampleURL); + test_delegate()->ChangeDSEOrigin(kExampleURL); EXPECT_EQ(CONTENT_SETTING_ALLOW, GetContentSetting(kGoogleURL, type)); // DSE setting initialized to true if the content setting is ASK. - test_delegate()->SetDSEOrigin(kGoogleURL); + test_delegate()->ChangeDSEOrigin(kGoogleURL); SetContentSetting(kGoogleURL, type, CONTENT_SETTING_DEFAULT); EXPECT_EQ(CONTENT_SETTING_ASK, GetContentSetting(kGoogleURL, type)); ReinitializeService(true /* clear_pref */); EXPECT_EQ(CONTENT_SETTING_ALLOW, GetContentSetting(kGoogleURL, type)); - test_delegate()->SetDSEOrigin(kExampleURL); + test_delegate()->ChangeDSEOrigin(kExampleURL); EXPECT_EQ(CONTENT_SETTING_ASK, GetContentSetting(kGoogleURL, type)); // DSE setting initialized to false if the content setting is BLOCK. - test_delegate()->SetDSEOrigin(kGoogleURL); + test_delegate()->ChangeDSEOrigin(kGoogleURL); SetContentSetting(kGoogleURL, type, CONTENT_SETTING_BLOCK); ReinitializeService(true /* clear_pref */); EXPECT_EQ(CONTENT_SETTING_BLOCK, GetContentSetting(kGoogleURL, type)); - test_delegate()->SetDSEOrigin(kExampleURL); + test_delegate()->ChangeDSEOrigin(kExampleURL); EXPECT_EQ(CONTENT_SETTING_BLOCK, GetContentSetting(kGoogleURL, type)); // Nothing happens if the pref is already set when the service is // initialized. - test_delegate()->SetDSEOrigin(kGoogleURL); + test_delegate()->ChangeDSEOrigin(kGoogleURL); SetContentSetting(kGoogleURL, type, CONTENT_SETTING_DEFAULT); ReinitializeService(false /* clear_pref */); EXPECT_EQ(CONTENT_SETTING_ASK, GetContentSetting(kGoogleURL, type)); @@ -233,7 +237,7 @@ TEST_F(SearchPermissionsServiceTest, Migration) { // When location was previously allowed for the DSE, it should be carried // over. - test_delegate()->SetDSEOrigin(kGoogleURL); + test_delegate()->ChangeDSEOrigin(kGoogleURL); EXPECT_EQ(CONTENT_SETTING_ALLOW, GetContentSetting(kGoogleURL, CONTENT_SETTINGS_TYPE_NOTIFICATIONS)); SetContentSetting(kGoogleURL, CONTENT_SETTINGS_TYPE_GEOLOCATION, @@ -260,7 +264,7 @@ EXPECT_EQ(CONTENT_SETTING_ALLOW, GetContentSetting(kGoogleURL, CONTENT_SETTINGS_TYPE_NOTIFICATIONS)); // Changing DSE should cause the setting to go back to ASK for Google. - test_delegate()->SetDSEOrigin(kExampleURL); + test_delegate()->ChangeDSEOrigin(kExampleURL); EXPECT_EQ(CONTENT_SETTING_ASK, GetContentSetting(kGoogleURL, CONTENT_SETTINGS_TYPE_GEOLOCATION)); @@ -281,7 +285,7 @@ TEST_F(SearchPermissionsServiceTest, ArePermissionsControlledByDSE) { // True for origin that matches the CCTLD and meets all requirements. - test_delegate()->SetDSEOrigin(kGoogleURL); + test_delegate()->ChangeDSEOrigin(kGoogleURL); EXPECT_TRUE( GetService()->ArePermissionsControlledByDSE(ToOrigin(kGoogleURL))); @@ -290,18 +294,18 @@ GetService()->ArePermissionsControlledByDSE(ToOrigin(kGoogleAusURL))); // False for http origin. - test_delegate()->SetDSEOrigin(kGoogleHTTPURL); + test_delegate()->ChangeDSEOrigin(kGoogleHTTPURL); EXPECT_FALSE( GetService()->ArePermissionsControlledByDSE(ToOrigin(kGoogleHTTPURL))); // True even for non-Google search engines. - test_delegate()->SetDSEOrigin(kExampleURL); + test_delegate()->ChangeDSEOrigin(kExampleURL); EXPECT_TRUE( GetService()->ArePermissionsControlledByDSE(ToOrigin(kExampleURL))); } TEST_F(SearchPermissionsServiceTest, DSEChanges) { - test_delegate()->SetDSEOrigin(kGoogleURL); + test_delegate()->ChangeDSEOrigin(kGoogleURL); EXPECT_EQ(CONTENT_SETTING_ALLOW, GetContentSetting(kGoogleURL, CONTENT_SETTINGS_TYPE_GEOLOCATION)); EXPECT_EQ(CONTENT_SETTING_ALLOW, @@ -309,7 +313,7 @@ // Change to google.com.au. Settings for google.com should revert and settings // for google.com.au should be set to allow. - test_delegate()->SetDSEOrigin(kGoogleAusURL); + test_delegate()->ChangeDSEOrigin(kGoogleAusURL); EXPECT_EQ(CONTENT_SETTING_ASK, GetContentSetting(kGoogleURL, CONTENT_SETTINGS_TYPE_GEOLOCATION)); EXPECT_EQ(CONTENT_SETTING_ASK, @@ -325,7 +329,7 @@ // change back to google.com, the setting should still be blocked. SetContentSetting(kGoogleURL, CONTENT_SETTINGS_TYPE_NOTIFICATIONS, CONTENT_SETTING_BLOCK); - test_delegate()->SetDSEOrigin(kGoogleURL); + test_delegate()->ChangeDSEOrigin(kGoogleURL); EXPECT_EQ(CONTENT_SETTING_BLOCK, GetContentSetting(kGoogleURL, CONTENT_SETTINGS_TYPE_NOTIFICATIONS)); EXPECT_EQ( @@ -337,7 +341,7 @@ // notifications setting should remain blocked. SetContentSetting(kGoogleAusURL, CONTENT_SETTINGS_TYPE_NOTIFICATIONS, CONTENT_SETTING_ALLOW); - test_delegate()->SetDSEOrigin(kGoogleAusURL); + test_delegate()->ChangeDSEOrigin(kGoogleAusURL); EXPECT_EQ(CONTENT_SETTING_BLOCK, GetContentSetting(kGoogleURL, CONTENT_SETTINGS_TYPE_NOTIFICATIONS)); EXPECT_EQ( @@ -346,7 +350,7 @@ // Now changing back to google.com, the google.com.au notifications setting // should be reset to ask (we reset it because of the conflict previously). - test_delegate()->SetDSEOrigin(kGoogleURL); + test_delegate()->ChangeDSEOrigin(kGoogleURL); EXPECT_EQ(CONTENT_SETTING_BLOCK, GetContentSetting(kGoogleURL, CONTENT_SETTINGS_TYPE_NOTIFICATIONS)); EXPECT_EQ( @@ -358,7 +362,7 @@ // reset it back to ask once it is no longer the DSE. SetContentSetting(kGoogleURL, CONTENT_SETTINGS_TYPE_NOTIFICATIONS, CONTENT_SETTING_ALLOW); - test_delegate()->SetDSEOrigin(kGoogleAusURL); + test_delegate()->ChangeDSEOrigin(kGoogleAusURL); EXPECT_EQ(CONTENT_SETTING_ASK, GetContentSetting(kGoogleURL, CONTENT_SETTINGS_TYPE_NOTIFICATIONS)); EXPECT_EQ( @@ -367,29 +371,29 @@ } TEST_F(SearchPermissionsServiceTest, DSEChangesAndDisclosure) { - test_delegate()->SetDSEOrigin(kGoogleURL); + test_delegate()->ChangeDSEOrigin(kGoogleURL); SearchGeolocationDisclosureTabHelper::FakeShowingDisclosureForTests( profile()); // Change to google.com.au. The disclosure should not be reset. - test_delegate()->SetDSEOrigin(kGoogleAusURL); + test_delegate()->ChangeDSEOrigin(kGoogleAusURL); EXPECT_FALSE(SearchGeolocationDisclosureTabHelper::IsDisclosureResetForTests( profile())); // Now set to a non-google search. The disclosure should be reset. - test_delegate()->SetDSEOrigin(kExampleURL); + test_delegate()->ChangeDSEOrigin(kExampleURL); EXPECT_TRUE(SearchGeolocationDisclosureTabHelper::IsDisclosureResetForTests( profile())); SearchGeolocationDisclosureTabHelper::FakeShowingDisclosureForTests( profile()); // Go back to google.com.au. The disclosure should again be reset. - test_delegate()->SetDSEOrigin(kGoogleAusURL); + test_delegate()->ChangeDSEOrigin(kGoogleAusURL); EXPECT_TRUE(SearchGeolocationDisclosureTabHelper::IsDisclosureResetForTests( profile())); } TEST_F(SearchPermissionsServiceTest, Embargo) { - test_delegate()->SetDSEOrigin(kGoogleURL); + test_delegate()->ChangeDSEOrigin(kGoogleURL); // Place another origin under embargo. GURL google_aus_url(kGoogleAusURL); @@ -407,9 +411,93 @@ EXPECT_EQ(result.content_setting, CONTENT_SETTING_BLOCK); // Now change the DSE to this origin and make sure the embargo is cleared. - test_delegate()->SetDSEOrigin(kGoogleAusURL); + test_delegate()->ChangeDSEOrigin(kGoogleAusURL); result = auto_blocker->GetEmbargoResult(GURL(kGoogleAusURL), CONTENT_SETTINGS_TYPE_GEOLOCATION); EXPECT_EQ(result.source, PermissionStatusSource::UNSPECIFIED); EXPECT_EQ(result.content_setting, CONTENT_SETTING_ASK); } + +TEST_F(SearchPermissionsServiceTest, DSEChangedButDisabled) { + SetContentSetting(kGoogleAusURL, CONTENT_SETTINGS_TYPE_GEOLOCATION, + CONTENT_SETTING_BLOCK); + + test_delegate()->ChangeDSEOrigin(kGoogleAusURL); + EXPECT_EQ( + CONTENT_SETTING_BLOCK, + GetContentSetting(kGoogleAusURL, CONTENT_SETTINGS_TYPE_GEOLOCATION)); + EXPECT_EQ( + CONTENT_SETTING_ALLOW, + GetContentSetting(kGoogleAusURL, CONTENT_SETTINGS_TYPE_NOTIFICATIONS)); + + // DSE disabled by enterprise policy + test_delegate()->ChangeDSEOrigin(std::string()); + + // The settings should return to their original value. + EXPECT_EQ( + CONTENT_SETTING_BLOCK, + GetContentSetting(kGoogleAusURL, CONTENT_SETTINGS_TYPE_GEOLOCATION)); + EXPECT_EQ( + CONTENT_SETTING_ASK, + GetContentSetting(kGoogleAusURL, CONTENT_SETTINGS_TYPE_NOTIFICATIONS)); + + // Now disable enterprise policy. We revert to the default ALLOW behavior. + test_delegate()->ChangeDSEOrigin(kGoogleAusURL); + EXPECT_EQ( + CONTENT_SETTING_BLOCK, + GetContentSetting(kGoogleAusURL, CONTENT_SETTINGS_TYPE_GEOLOCATION)); + EXPECT_EQ( + CONTENT_SETTING_ALLOW, + GetContentSetting(kGoogleAusURL, CONTENT_SETTINGS_TYPE_NOTIFICATIONS)); +} + +TEST_F(SearchPermissionsServiceTest, DSEInitializedButDisabled) { + test_delegate()->ChangeDSEOrigin(kGoogleAusURL); + EXPECT_EQ( + CONTENT_SETTING_ALLOW, + GetContentSetting(kGoogleAusURL, CONTENT_SETTINGS_TYPE_GEOLOCATION)); + EXPECT_EQ( + CONTENT_SETTING_ALLOW, + GetContentSetting(kGoogleAusURL, CONTENT_SETTINGS_TYPE_NOTIFICATIONS)); + + // Set the DSE origin without calling OnDSEChanged. + test_delegate()->set_dse_origin(std::string()); + + ReinitializeService(/*clear_pref=*/false); + + // Settings should revert back to default. + EXPECT_EQ( + CONTENT_SETTING_ASK, + GetContentSetting(kGoogleAusURL, CONTENT_SETTINGS_TYPE_GEOLOCATION)); + EXPECT_EQ( + CONTENT_SETTING_ASK, + GetContentSetting(kGoogleAusURL, CONTENT_SETTINGS_TYPE_NOTIFICATIONS)); + + // The pref shouldn't exist anymore. + EXPECT_FALSE( + profile()->GetPrefs()->HasPrefPath(prefs::kDSEPermissionsSettings)); + + // Firing the DSE changed event now should not do anything. + test_delegate()->ChangeDSEOrigin(std::string()); + + EXPECT_EQ( + CONTENT_SETTING_ASK, + GetContentSetting(kGoogleAusURL, CONTENT_SETTINGS_TYPE_GEOLOCATION)); + EXPECT_EQ( + CONTENT_SETTING_ASK, + GetContentSetting(kGoogleAusURL, CONTENT_SETTINGS_TYPE_NOTIFICATIONS)); + + EXPECT_FALSE( + profile()->GetPrefs()->HasPrefPath(prefs::kDSEPermissionsSettings)); + + // Re-enabling the DSE origin should revert to the default ALLOW behavior. + test_delegate()->set_dse_origin(kGoogleAusURL); + ReinitializeService(/*clear_pref=*/false); + + EXPECT_EQ( + CONTENT_SETTING_ALLOW, + GetContentSetting(kGoogleAusURL, CONTENT_SETTINGS_TYPE_GEOLOCATION)); + EXPECT_EQ( + CONTENT_SETTING_ALLOW, + GetContentSetting(kGoogleAusURL, CONTENT_SETTINGS_TYPE_NOTIFICATIONS)); +}
diff --git a/content/browser/devtools/shared_worker_devtools_agent_host.cc b/content/browser/devtools/shared_worker_devtools_agent_host.cc index 4e326578..91fd27e5 100644 --- a/content/browser/devtools/shared_worker_devtools_agent_host.cc +++ b/content/browser/devtools/shared_worker_devtools_agent_host.cc
@@ -4,31 +4,48 @@ #include "content/browser/devtools/shared_worker_devtools_agent_host.h" +#include "content/browser/devtools/devtools_session.h" +#include "content/browser/devtools/protocol/inspector_handler.h" +#include "content/browser/devtools/protocol/network_handler.h" +#include "content/browser/devtools/protocol/protocol.h" +#include "content/browser/devtools/protocol/schema_handler.h" #include "content/browser/devtools/shared_worker_devtools_manager.h" +#include "content/browser/shared_worker/shared_worker_host.h" #include "content/browser/shared_worker/shared_worker_instance.h" #include "content/browser/shared_worker/shared_worker_service_impl.h" #include "content/public/browser/browser_thread.h" +#include "content/public/browser/render_process_host.h" namespace content { SharedWorkerDevToolsAgentHost::SharedWorkerDevToolsAgentHost( - WorkerId worker_id, - const SharedWorkerInstance& shared_worker) - : WorkerDevToolsAgentHost(shared_worker.devtools_worker_token(), worker_id), - shared_worker_(new SharedWorkerInstance(shared_worker)) { + SharedWorkerHost* worker_host) + : DevToolsAgentHostImpl( + worker_host->instance()->devtools_worker_token().ToString()), + worker_host_(worker_host), + instance_(new SharedWorkerInstance(*worker_host->instance())) { NotifyCreated(); } +SharedWorkerDevToolsAgentHost::~SharedWorkerDevToolsAgentHost() { + SharedWorkerDevToolsManager::GetInstance()->AgentHostDestroyed(this); +} + +BrowserContext* SharedWorkerDevToolsAgentHost::GetBrowserContext() { + RenderProcessHost* rph = GetProcess(); + return rph ? rph->GetBrowserContext() : nullptr; +} + std::string SharedWorkerDevToolsAgentHost::GetType() { return kTypeSharedWorker; } std::string SharedWorkerDevToolsAgentHost::GetTitle() { - return shared_worker_->name(); + return instance_->name(); } GURL SharedWorkerDevToolsAgentHost::GetURL() { - return shared_worker_->url(); + return instance_->url(); } bool SharedWorkerDevToolsAgentHost::Activate() { @@ -39,19 +56,128 @@ } bool SharedWorkerDevToolsAgentHost::Close() { - static_cast<SharedWorkerServiceImpl*>(SharedWorkerService::GetInstance()) - ->TerminateWorkerById(worker_id().first, worker_id().second); + if (worker_host_) + worker_host_->TerminateWorker(); return true; } -bool SharedWorkerDevToolsAgentHost::Matches( - const SharedWorkerInstance& other) { - return shared_worker_->Matches(other); +void SharedWorkerDevToolsAgentHost::AttachSession(DevToolsSession* session) { + if (RenderProcessHost* host = GetProcess()) { + if (sessions().size() == 1) + host->AddRoute(worker_host_->route_id(), this); + session->SetRenderer(host, nullptr); + if (!waiting_ready_for_reattach_) { + host->Send(new DevToolsAgentMsg_Attach(worker_host_->route_id(), + session->session_id())); + } + } + session->SetFallThroughForNotFound(true); + session->AddHandler(std::make_unique<protocol::InspectorHandler>()); + session->AddHandler(std::make_unique<protocol::NetworkHandler>(GetId())); + session->AddHandler(std::make_unique<protocol::SchemaHandler>()); } -SharedWorkerDevToolsAgentHost::~SharedWorkerDevToolsAgentHost() { - SharedWorkerDevToolsManager::GetInstance()->RemoveInspectedWorkerData( - worker_id()); +void SharedWorkerDevToolsAgentHost::DetachSession(int session_id) { + if (RenderProcessHost* host = GetProcess()) { + host->Send( + new DevToolsAgentMsg_Detach(worker_host_->route_id(), session_id)); + if (!sessions().size()) + host->RemoveRoute(worker_host_->route_id()); + } +} + +bool SharedWorkerDevToolsAgentHost::DispatchProtocolMessage( + DevToolsSession* session, + const std::string& message) { + int call_id = 0; + std::string method; + if (session->Dispatch(message, &call_id, &method) != + protocol::Response::kFallThrough) { + return true; + } + + if (RenderProcessHost* host = GetProcess()) { + host->Send(new DevToolsAgentMsg_DispatchOnInspectorBackend( + worker_host_->route_id(), session->session_id(), call_id, method, + message)); + session->waiting_messages()[call_id] = {method, message}; + } + return true; +} + +bool SharedWorkerDevToolsAgentHost::OnMessageReceived(const IPC::Message& msg) { + DCHECK_CURRENTLY_ON(BrowserThread::UI); + bool handled = true; + IPC_BEGIN_MESSAGE_MAP(SharedWorkerDevToolsAgentHost, msg) + IPC_MESSAGE_HANDLER(DevToolsClientMsg_DispatchOnInspectorFrontend, + OnDispatchOnInspectorFrontend) + IPC_MESSAGE_UNHANDLED(handled = false) + IPC_END_MESSAGE_MAP() + return handled; +} + +bool SharedWorkerDevToolsAgentHost::Matches(SharedWorkerHost* worker_host) { + return instance_->Matches(*worker_host->instance()); +} + +void SharedWorkerDevToolsAgentHost::WorkerReadyForInspection() { + DCHECK(worker_host_); + if (!waiting_ready_for_reattach_) + return; + waiting_ready_for_reattach_ = false; + if (RenderProcessHost* host = GetProcess()) { + for (DevToolsSession* session : sessions()) { + host->Send(new DevToolsAgentMsg_Reattach(worker_host_->route_id(), + session->session_id(), + session->state_cookie())); + for (const auto& pair : session->waiting_messages()) { + int call_id = pair.first; + const DevToolsSession::Message& message = pair.second; + host->Send(new DevToolsAgentMsg_DispatchOnInspectorBackend( + worker_host_->route_id(), session->session_id(), call_id, + message.method, message.message)); + } + } + } +} + +bool SharedWorkerDevToolsAgentHost::WorkerRestarted( + SharedWorkerHost* worker_host) { + DCHECK(!worker_host_); + worker_host_ = worker_host; + if (RenderProcessHost* host = GetProcess()) { + if (sessions().size()) + host->AddRoute(worker_host_->route_id(), this); + for (DevToolsSession* session : sessions()) + session->SetRenderer(host, nullptr); + } + waiting_ready_for_reattach_ = IsAttached(); + return waiting_ready_for_reattach_; +} + +void SharedWorkerDevToolsAgentHost::WorkerDestroyed() { + DCHECK(worker_host_); + for (auto* inspector : protocol::InspectorHandler::ForAgentHost(this)) + inspector->TargetCrashed(); + for (DevToolsSession* session : sessions()) + session->SetRenderer(nullptr, nullptr); + if (sessions().size()) { + if (RenderProcessHost* host = GetProcess()) + host->RemoveRoute(worker_host_->route_id()); + } + worker_host_ = nullptr; +} + +RenderProcessHost* SharedWorkerDevToolsAgentHost::GetProcess() { + return worker_host_ ? RenderProcessHost::FromID(worker_host_->process_id()) + : nullptr; +} + +void SharedWorkerDevToolsAgentHost::OnDispatchOnInspectorFrontend( + const DevToolsMessageChunk& message) { + DevToolsSession* session = SessionById(message.session_id); + if (session) + session->ReceiveMessageChunk(message); } } // namespace content
diff --git a/content/browser/devtools/shared_worker_devtools_agent_host.h b/content/browser/devtools/shared_worker_devtools_agent_host.h index 1566798..c445bc9 100644 --- a/content/browser/devtools/shared_worker_devtools_agent_host.h +++ b/content/browser/devtools/shared_worker_devtools_agent_host.h
@@ -6,20 +6,24 @@ #define CONTENT_BROWSER_DEVTOOLS_SHARED_WORKER_DEVTOOLS_AGENT_HOST_H_ #include "base/macros.h" -#include "content/browser/devtools/worker_devtools_agent_host.h" +#include "content/browser/devtools/devtools_agent_host_impl.h" +#include "ipc/ipc_listener.h" namespace content { class SharedWorkerInstance; +class SharedWorkerHost; +class RenderProcessHost; -class SharedWorkerDevToolsAgentHost : public WorkerDevToolsAgentHost { +class SharedWorkerDevToolsAgentHost : public DevToolsAgentHostImpl, + public IPC::Listener { public: using List = std::vector<scoped_refptr<SharedWorkerDevToolsAgentHost>>; - SharedWorkerDevToolsAgentHost(WorkerId worker_id, - const SharedWorkerInstance& shared_worker); + explicit SharedWorkerDevToolsAgentHost(SharedWorkerHost* worker_host); // DevToolsAgentHost override. + BrowserContext* GetBrowserContext() override; std::string GetType() override; std::string GetTitle() override; GURL GetURL() override; @@ -27,13 +31,31 @@ void Reload() override; bool Close() override; - bool Matches(const SharedWorkerInstance& other); + // DevToolsAgentHostImpl overrides. + void AttachSession(DevToolsSession* session) override; + void DetachSession(int session_id) override; + bool DispatchProtocolMessage(DevToolsSession* session, + const std::string& message) override; + + // IPC::Listener implementation. + bool OnMessageReceived(const IPC::Message& msg) override; + + bool Matches(SharedWorkerHost* worker_host); + void WorkerReadyForInspection(); + // Returns whether the worker should be paused for reattach. + bool WorkerRestarted(SharedWorkerHost* worker_host); + void WorkerDestroyed(); private: friend class SharedWorkerDevToolsManagerTest; ~SharedWorkerDevToolsAgentHost() override; - std::unique_ptr<SharedWorkerInstance> shared_worker_; + RenderProcessHost* GetProcess(); + void OnDispatchOnInspectorFrontend(const DevToolsMessageChunk& message); + + SharedWorkerHost* worker_host_; + std::unique_ptr<SharedWorkerInstance> instance_; + bool waiting_ready_for_reattach_ = false; DISALLOW_COPY_AND_ASSIGN(SharedWorkerDevToolsAgentHost); };
diff --git a/content/browser/devtools/shared_worker_devtools_manager.cc b/content/browser/devtools/shared_worker_devtools_manager.cc index 9903398..911c816 100644 --- a/content/browser/devtools/shared_worker_devtools_manager.cc +++ b/content/browser/devtools/shared_worker_devtools_manager.cc
@@ -5,7 +5,7 @@ #include "content/browser/devtools/shared_worker_devtools_manager.h" #include "content/browser/devtools/shared_worker_devtools_agent_host.h" -#include "content/browser/shared_worker/shared_worker_instance.h" +#include "content/browser/shared_worker/shared_worker_host.h" #include "content/public/browser/browser_thread.h" namespace content { @@ -18,80 +18,56 @@ void SharedWorkerDevToolsManager::AddAllAgentHosts( SharedWorkerDevToolsAgentHost::List* result) { - for (auto& worker : workers_) { - if (!worker.second->IsTerminated()) - result->push_back(worker.second); - } + for (auto& it : live_hosts_) + result->push_back(it.second.get()); } -bool SharedWorkerDevToolsManager::WorkerCreated( - int worker_process_id, - int worker_route_id, - const SharedWorkerInstance& instance) { +bool SharedWorkerDevToolsManager::WorkerCreated(SharedWorkerHost* worker_host) { DCHECK_CURRENTLY_ON(BrowserThread::UI); - const WorkerId id(worker_process_id, worker_route_id); - AgentHostMap::iterator it = - FindExistingWorkerAgentHost(instance); - if (it == workers_.end()) { - workers_[id] = new SharedWorkerDevToolsAgentHost(id, instance); + DCHECK(live_hosts_.find(worker_host) == live_hosts_.end()); + + auto it = + std::find_if(terminated_hosts_.begin(), terminated_hosts_.end(), + [&worker_host](SharedWorkerDevToolsAgentHost* agent_host) { + return agent_host->Matches(worker_host); + }); + if (it == terminated_hosts_.end()) { + live_hosts_[worker_host] = new SharedWorkerDevToolsAgentHost(worker_host); return false; } - // Worker restarted. - SharedWorkerDevToolsAgentHost* agent_host = it->second; - agent_host->WorkerRestarted(id); - workers_.erase(it); - workers_[id] = agent_host; - return agent_host->IsAttached(); + SharedWorkerDevToolsAgentHost* agent_host = *it; + terminated_hosts_.erase(it); + live_hosts_[worker_host] = agent_host; + return agent_host->WorkerRestarted(worker_host); } void SharedWorkerDevToolsManager::WorkerReadyForInspection( - int worker_process_id, - int worker_route_id) { + SharedWorkerHost* worker_host) { DCHECK_CURRENTLY_ON(BrowserThread::UI); - const WorkerId id(worker_process_id, worker_route_id); - AgentHostMap::iterator it = workers_.find(id); - if (it == workers_.end() || it->second->IsTerminated()) - return; - it->second->WorkerReadyForInspection(); + live_hosts_[worker_host]->WorkerReadyForInspection(); } void SharedWorkerDevToolsManager::WorkerDestroyed( - int worker_process_id, - int worker_route_id) { + SharedWorkerHost* worker_host) { DCHECK_CURRENTLY_ON(BrowserThread::UI); - const WorkerId id(worker_process_id, worker_route_id); - AgentHostMap::iterator it = workers_.find(id); - if (it == workers_.end() || it->second->IsTerminated()) - return; - scoped_refptr<SharedWorkerDevToolsAgentHost> agent_host(it->second); + scoped_refptr<SharedWorkerDevToolsAgentHost> agent_host = + live_hosts_[worker_host]; + live_hosts_.erase(worker_host); + terminated_hosts_.insert(agent_host.get()); agent_host->WorkerDestroyed(); } -void SharedWorkerDevToolsManager::RemoveInspectedWorkerData(WorkerId id) { +void SharedWorkerDevToolsManager::AgentHostDestroyed( + SharedWorkerDevToolsAgentHost* agent_host) { DCHECK_CURRENTLY_ON(BrowserThread::UI); - workers_.erase(id); + terminated_hosts_.erase(agent_host); } + SharedWorkerDevToolsManager::SharedWorkerDevToolsManager() { } SharedWorkerDevToolsManager::~SharedWorkerDevToolsManager() { } -SharedWorkerDevToolsManager::AgentHostMap::iterator -SharedWorkerDevToolsManager::FindExistingWorkerAgentHost( - const SharedWorkerInstance& instance) { - AgentHostMap::iterator it = workers_.begin(); - for (; it != workers_.end(); ++it) { - if (it->second->Matches(instance)) - break; - } - return it; -} - -void SharedWorkerDevToolsManager::ResetForTesting() { - workers_.clear(); -} - - } // namespace content
diff --git a/content/browser/devtools/shared_worker_devtools_manager.h b/content/browser/devtools/shared_worker_devtools_manager.h index c7f4f7c..551d2ef 100644 --- a/content/browser/devtools/shared_worker_devtools_manager.h +++ b/content/browser/devtools/shared_worker_devtools_manager.h
@@ -7,6 +7,7 @@ #include <map> +#include "base/containers/flat_set.h" #include "base/gtest_prod_util.h" #include "base/macros.h" #include "base/memory/singleton.h" @@ -15,14 +16,12 @@ namespace content { class SharedWorkerDevToolsAgentHost; -class SharedWorkerInstance; +class SharedWorkerHost; // Manages WorkerDevToolsAgentHost's for Shared Workers. // This class lives on UI thread. class CONTENT_EXPORT SharedWorkerDevToolsManager { public: - using WorkerId = std::pair<int, int>; - // Returns the SharedWorkerDevToolsManager singleton. static SharedWorkerDevToolsManager* GetInstance(); @@ -31,12 +30,9 @@ // Returns true when the worker must be paused on start because a DevTool // window for the same former SharedWorkerInstance is still opened. - bool WorkerCreated(int worker_process_id, - int worker_route_id, - const SharedWorkerInstance& instance); - void WorkerReadyForInspection(int worker_process_id, int worker_route_id); - void WorkerDestroyed(int worker_process_id, int worker_route_id); - void RemoveInspectedWorkerData(WorkerId id); + bool WorkerCreated(SharedWorkerHost* worker_host); + void WorkerReadyForInspection(SharedWorkerHost* worker_host); + void WorkerDestroyed(SharedWorkerHost* worker_host); private: friend struct base::DefaultSingletonTraits<SharedWorkerDevToolsManager>; @@ -45,18 +41,17 @@ FRIEND_TEST_ALL_PREFIXES(SharedWorkerDevToolsManagerTest, BasicTest); FRIEND_TEST_ALL_PREFIXES(SharedWorkerDevToolsManagerTest, AttachTest); - using AgentHostMap = std::map<WorkerId, SharedWorkerDevToolsAgentHost*>; - SharedWorkerDevToolsManager(); ~SharedWorkerDevToolsManager(); + void AgentHostDestroyed(SharedWorkerDevToolsAgentHost* agent_host); - AgentHostMap::iterator FindExistingWorkerAgentHost( - const SharedWorkerInstance& instance); + // We retatin agent hosts as long as the shared worker is alive. + std::map<SharedWorkerHost*, scoped_refptr<SharedWorkerDevToolsAgentHost>> + live_hosts_; + // Clients may retain agent host for the terminated shared worker, + // and we reconnect them when shared worker is restarted. + base::flat_set<SharedWorkerDevToolsAgentHost*> terminated_hosts_; - // Resets to its initial state as if newly created. - void ResetForTesting(); - - AgentHostMap workers_; DISALLOW_COPY_AND_ASSIGN(SharedWorkerDevToolsManager); };
diff --git a/content/browser/devtools/shared_worker_devtools_manager_unittest.cc b/content/browser/devtools/shared_worker_devtools_manager_unittest.cc deleted file mode 100644 index 66c5747..0000000 --- a/content/browser/devtools/shared_worker_devtools_manager_unittest.cc +++ /dev/null
@@ -1,349 +0,0 @@ -// Copyright 2014 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/devtools/shared_worker_devtools_manager.h" - -#include <stddef.h> - -#include <memory> - -#include "base/macros.h" -#include "base/run_loop.h" -#include "content/browser/browser_thread_impl.h" -#include "content/browser/devtools/devtools_agent_host_impl.h" -#include "content/browser/devtools/shared_worker_devtools_agent_host.h" -#include "content/browser/devtools/worker_devtools_agent_host.h" -#include "content/browser/shared_worker/shared_worker_instance.h" -#include "content/browser/shared_worker/worker_storage_partition.h" -#include "content/public/browser/browser_context.h" -#include "content/public/browser/storage_partition.h" -#include "content/public/test/test_browser_context.h" -#include "content/public/test/test_browser_thread_bundle.h" -#include "testing/gtest/include/gtest/gtest.h" - -namespace content { -namespace { - -class TestDevToolsClientHost : public DevToolsAgentHostClient { - public: - TestDevToolsClientHost() {} - ~TestDevToolsClientHost() override {} - void DispatchProtocolMessage(DevToolsAgentHost* agent_host, - const std::string& message) override {} - void AgentHostClosed(DevToolsAgentHost* agent_host) override {} - - void InspectAgentHost(DevToolsAgentHost* agent_host) { - if (agent_host_.get()) - agent_host_->DetachClient(this); - agent_host_ = agent_host; - if (agent_host_.get()) - agent_host_->AttachClient(this); - } - private: - scoped_refptr<DevToolsAgentHost> agent_host_; - DISALLOW_COPY_AND_ASSIGN(TestDevToolsClientHost); -}; -} // namespace - -class SharedWorkerDevToolsManagerTest : public testing::Test { - public: - typedef SharedWorkerDevToolsAgentHost::WorkerState WorkerState; - - SharedWorkerDevToolsManagerTest() - : browser_thread_bundle_(TestBrowserThreadBundle::IO_MAINLOOP), - browser_context_(new TestBrowserContext()), - partition_(new WorkerStoragePartition( - BrowserContext::GetDefaultStoragePartition(browser_context_.get()) - ->GetURLRequestContext(), - nullptr, - nullptr, - nullptr, - nullptr, - nullptr, - nullptr, - nullptr)), - partition_id_(*partition_.get()) {} - - protected: - void SetUp() override { - manager_ = SharedWorkerDevToolsManager::GetInstance(); - } - void TearDown() override { - SharedWorkerDevToolsManager::GetInstance()->ResetForTesting(); - } - - void CheckWorkerState(int worker_process_id, - int worker_route_id, - WorkerState state) { - SharedWorkerDevToolsAgentHost* host = - GetDevToolsAgentHostForWorker(worker_process_id, worker_route_id); - EXPECT_TRUE(!!host); - EXPECT_EQ(state, host->state_); - } - - void CheckWorkerNotExist(int worker_process_id, int worker_route_id) { - EXPECT_TRUE( - !GetDevToolsAgentHostForWorker(worker_process_id, worker_route_id)); - } - - void CheckWorkerCount(size_t size) { - EXPECT_EQ(size, manager_->workers_.size()); - } - - SharedWorkerDevToolsAgentHost* GetDevToolsAgentHostForWorker( - int worker_process_id, - int worker_route_id) { - const SharedWorkerDevToolsManager::WorkerId id(worker_process_id, - worker_route_id); - auto it = manager_->workers_.find(id); - return it == manager_->workers_.end() ? nullptr : it->second; - } - - TestBrowserThreadBundle browser_thread_bundle_; - std::unique_ptr<TestBrowserContext> browser_context_; - std::unique_ptr<WorkerStoragePartition> partition_; - const WorkerStoragePartitionId partition_id_; - SharedWorkerDevToolsManager* manager_; -}; - -TEST_F(SharedWorkerDevToolsManagerTest, BasicTest) { - scoped_refptr<DevToolsAgentHostImpl> agent_host; - - SharedWorkerInstance instance1( - GURL("http://example.com/w.js"), std::string(), - url::Origin::Create(GURL("http://example.com/")), std::string(), - blink::kWebContentSecurityPolicyTypeReport, blink::kWebAddressSpacePublic, - browser_context_->GetResourceContext(), partition_id_, - blink::mojom::SharedWorkerCreationContextType::kNonsecure, - base::UnguessableToken::Create()); - - agent_host = GetDevToolsAgentHostForWorker(1, 1); - EXPECT_FALSE(agent_host.get()); - - // Created -> Started -> Destroyed - CheckWorkerNotExist(1, 1); - manager_->WorkerCreated(1, 1, instance1); - CheckWorkerState(1, 1, WorkerState::WORKER_UNINSPECTED); - manager_->WorkerReadyForInspection(1, 1); - CheckWorkerState(1, 1, WorkerState::WORKER_UNINSPECTED); - manager_->WorkerDestroyed(1, 1); - CheckWorkerNotExist(1, 1); - - // Created -> GetDevToolsAgentHost -> Started -> Destroyed - CheckWorkerNotExist(1, 2); - manager_->WorkerCreated(1, 2, instance1); - CheckWorkerState(1, 2, WorkerState::WORKER_UNINSPECTED); - agent_host = GetDevToolsAgentHostForWorker(1, 2); - EXPECT_TRUE(agent_host.get()); - CheckWorkerState(1, 2, WorkerState::WORKER_UNINSPECTED); - EXPECT_EQ(agent_host.get(), GetDevToolsAgentHostForWorker(1, 2)); - manager_->WorkerReadyForInspection(1, 2); - CheckWorkerState(1, 2, WorkerState::WORKER_UNINSPECTED); - manager_->WorkerDestroyed(1, 2); - CheckWorkerState(1, 2, WorkerState::WORKER_TERMINATED); - agent_host = nullptr; - CheckWorkerNotExist(1, 2); - - // Created -> Started -> GetDevToolsAgentHost -> Destroyed - CheckWorkerNotExist(1, 3); - manager_->WorkerCreated(1, 3, instance1); - CheckWorkerState(1, 3, WorkerState::WORKER_UNINSPECTED); - manager_->WorkerReadyForInspection(1, 3); - CheckWorkerState(1, 3, WorkerState::WORKER_UNINSPECTED); - agent_host = GetDevToolsAgentHostForWorker(1, 3); - EXPECT_TRUE(agent_host.get()); - CheckWorkerState(1, 3, WorkerState::WORKER_UNINSPECTED); - manager_->WorkerDestroyed(1, 3); - CheckWorkerState(1, 3, WorkerState::WORKER_TERMINATED); - agent_host = nullptr; - CheckWorkerNotExist(1, 3); - - // Created -> Destroyed - CheckWorkerNotExist(1, 4); - manager_->WorkerCreated(1, 4, instance1); - CheckWorkerState(1, 4, WorkerState::WORKER_UNINSPECTED); - manager_->WorkerDestroyed(1, 4); - CheckWorkerNotExist(1, 4); - - // Created -> GetDevToolsAgentHost -> Destroyed - CheckWorkerNotExist(1, 5); - manager_->WorkerCreated(1, 5, instance1); - CheckWorkerState(1, 5, WorkerState::WORKER_UNINSPECTED); - agent_host = GetDevToolsAgentHostForWorker(1, 5); - EXPECT_TRUE(agent_host.get()); - CheckWorkerState(1, 5, WorkerState::WORKER_UNINSPECTED); - manager_->WorkerDestroyed(1, 5); - CheckWorkerState(1, 5, WorkerState::WORKER_TERMINATED); - agent_host = nullptr; - CheckWorkerNotExist(1, 5); - - // Created -> GetDevToolsAgentHost -> Free agent_host -> Destroyed - CheckWorkerNotExist(1, 6); - manager_->WorkerCreated(1, 6, instance1); - CheckWorkerState(1, 6, WorkerState::WORKER_UNINSPECTED); - agent_host = GetDevToolsAgentHostForWorker(1, 6); - EXPECT_TRUE(agent_host.get()); - CheckWorkerState(1, 6, WorkerState::WORKER_UNINSPECTED); - agent_host = nullptr; - manager_->WorkerDestroyed(1, 6); - CheckWorkerNotExist(1, 6); -} - -TEST_F(SharedWorkerDevToolsManagerTest, AttachTest) { - scoped_refptr<DevToolsAgentHostImpl> agent_host1; - scoped_refptr<DevToolsAgentHostImpl> agent_host2; - - SharedWorkerInstance instance1( - GURL("http://example.com/w1.js"), std::string(), - url::Origin::Create(GURL("http://example.com/")), std::string(), - blink::kWebContentSecurityPolicyTypeReport, blink::kWebAddressSpacePublic, - browser_context_->GetResourceContext(), partition_id_, - blink::mojom::SharedWorkerCreationContextType::kNonsecure, - base::UnguessableToken::Create()); - SharedWorkerInstance instance2( - GURL("http://example.com/w2.js"), std::string(), - url::Origin::Create(GURL("http://example.com/")), std::string(), - blink::kWebContentSecurityPolicyTypeReport, blink::kWebAddressSpacePublic, - browser_context_->GetResourceContext(), partition_id_, - blink::mojom::SharedWorkerCreationContextType::kNonsecure, - base::UnguessableToken::Create()); - - // Created -> GetDevToolsAgentHost -> Register -> Started -> Destroyed - std::unique_ptr<TestDevToolsClientHost> client_host1( - new TestDevToolsClientHost()); - CheckWorkerNotExist(2, 1); - manager_->WorkerCreated(2, 1, instance1); - CheckWorkerState(2, 1, WorkerState::WORKER_UNINSPECTED); - agent_host1 = GetDevToolsAgentHostForWorker(2, 1); - EXPECT_TRUE(agent_host1.get()); - CheckWorkerState(2, 1, WorkerState::WORKER_UNINSPECTED); - EXPECT_EQ(agent_host1.get(), GetDevToolsAgentHostForWorker(2, 1)); - client_host1->InspectAgentHost(agent_host1.get()); - CheckWorkerState(2, 1, WorkerState::WORKER_INSPECTED); - manager_->WorkerReadyForInspection(2, 1); - CheckWorkerState(2, 1, WorkerState::WORKER_INSPECTED); - manager_->WorkerDestroyed(2, 1); - CheckWorkerState(2, 1, WorkerState::WORKER_TERMINATED); - EXPECT_EQ(agent_host1.get(), GetDevToolsAgentHostForWorker(2, 1)); - - // Created -> Started -> GetDevToolsAgentHost -> Register -> Destroyed - std::unique_ptr<TestDevToolsClientHost> client_host2( - new TestDevToolsClientHost()); - manager_->WorkerCreated(2, 2, instance2); - CheckWorkerState(2, 2, WorkerState::WORKER_UNINSPECTED); - manager_->WorkerReadyForInspection(2, 2); - CheckWorkerState(2, 2, WorkerState::WORKER_UNINSPECTED); - agent_host2 = GetDevToolsAgentHostForWorker(2, 2); - EXPECT_TRUE(agent_host2.get()); - EXPECT_NE(agent_host1.get(), agent_host2.get()); - EXPECT_EQ(agent_host2.get(), GetDevToolsAgentHostForWorker(2, 2)); - CheckWorkerState(2, 2, WorkerState::WORKER_UNINSPECTED); - client_host2->InspectAgentHost(agent_host2.get()); - CheckWorkerState(2, 2, WorkerState::WORKER_INSPECTED); - manager_->WorkerDestroyed(2, 2); - CheckWorkerState(2, 2, WorkerState::WORKER_TERMINATED); - EXPECT_EQ(agent_host2.get(), GetDevToolsAgentHostForWorker(2, 2)); - - // Re-created -> Started -> ClientHostClosing -> Destroyed - CheckWorkerState(2, 1, WorkerState::WORKER_TERMINATED); - manager_->WorkerCreated(2, 3, instance1); - CheckWorkerNotExist(2, 1); - CheckWorkerState(2, 3, WorkerState::WORKER_PAUSED_FOR_REATTACH); - EXPECT_EQ(agent_host1.get(), GetDevToolsAgentHostForWorker(2, 3)); - manager_->WorkerReadyForInspection(2, 3); - CheckWorkerState(2, 3, WorkerState::WORKER_INSPECTED); - client_host1->InspectAgentHost(nullptr); - manager_->WorkerDestroyed(2, 3); - CheckWorkerState(2, 3, WorkerState::WORKER_TERMINATED); - agent_host1 = nullptr; - CheckWorkerNotExist(2, 3); - - // Re-created -> Destroyed - CheckWorkerState(2, 2, WorkerState::WORKER_TERMINATED); - manager_->WorkerCreated(2, 4, instance2); - CheckWorkerNotExist(2, 2); - CheckWorkerState(2, 4, WorkerState::WORKER_PAUSED_FOR_REATTACH); - EXPECT_EQ(agent_host2.get(), GetDevToolsAgentHostForWorker(2, 4)); - manager_->WorkerDestroyed(2, 4); - CheckWorkerNotExist(2, 2); - CheckWorkerState(2, 4, WorkerState::WORKER_TERMINATED); - - // Re-created -> ClientHostClosing -> Destroyed - manager_->WorkerCreated(2, 5, instance2); - CheckWorkerNotExist(2, 2); - CheckWorkerState(2, 5, WorkerState::WORKER_PAUSED_FOR_REATTACH); - EXPECT_EQ(agent_host2.get(), GetDevToolsAgentHostForWorker(2, 5)); - client_host2->InspectAgentHost(nullptr); - CheckWorkerCount(1); - agent_host2 = nullptr; - CheckWorkerCount(1); - manager_->WorkerDestroyed(2, 5); - CheckWorkerCount(0); -} - -TEST_F(SharedWorkerDevToolsManagerTest, ReattachTest) { - SharedWorkerInstance instance( - GURL("http://example.com/w3.js"), std::string(), - url::Origin::Create(GURL("http://example.com/")), std::string(), - blink::kWebContentSecurityPolicyTypeReport, blink::kWebAddressSpacePublic, - browser_context_->GetResourceContext(), partition_id_, - blink::mojom::SharedWorkerCreationContextType::kNonsecure, - base::UnguessableToken::Create()); - std::unique_ptr<TestDevToolsClientHost> client_host( - new TestDevToolsClientHost()); - // Created -> GetDevToolsAgentHost -> Register -> Destroyed - manager_->WorkerCreated(3, 1, instance); - CheckWorkerState(3, 1, WorkerState::WORKER_UNINSPECTED); - scoped_refptr<DevToolsAgentHost> agent_host( - GetDevToolsAgentHostForWorker(3, 1)); - EXPECT_TRUE(agent_host.get()); - CheckWorkerState(3, 1, WorkerState::WORKER_UNINSPECTED); - client_host->InspectAgentHost(agent_host.get()); - CheckWorkerState(3, 1, WorkerState::WORKER_INSPECTED); - manager_->WorkerDestroyed(3, 1); - CheckWorkerState(3, 1, WorkerState::WORKER_TERMINATED); - // ClientHostClosing -> Re-created -> release agent_host -> Destroyed - client_host->InspectAgentHost(nullptr); - CheckWorkerState(3, 1, WorkerState::WORKER_TERMINATED); - manager_->WorkerCreated(3, 2, instance); - CheckWorkerState(3, 2, WorkerState::WORKER_UNINSPECTED); - agent_host = nullptr; - CheckWorkerState(3, 2, WorkerState::WORKER_UNINSPECTED); - manager_->WorkerDestroyed(3, 2); - CheckWorkerNotExist(3, 2); - CheckWorkerCount(0); -} - -TEST_F(SharedWorkerDevToolsManagerTest, PauseOnStartTest) { - SharedWorkerInstance instance( - GURL("http://example.com/w3.js"), std::string(), - url::Origin::Create(GURL("http://example.com/")), std::string(), - blink::kWebContentSecurityPolicyTypeReport, blink::kWebAddressSpacePublic, - browser_context_->GetResourceContext(), partition_id_, - blink::mojom::SharedWorkerCreationContextType::kNonsecure, - base::UnguessableToken::Create()); - std::unique_ptr<TestDevToolsClientHost> client_host( - new TestDevToolsClientHost()); - manager_->WorkerCreated(3, 1, instance); - CheckWorkerState(3, 1, WorkerState::WORKER_UNINSPECTED); - scoped_refptr<SharedWorkerDevToolsAgentHost> agent_host( - GetDevToolsAgentHostForWorker(3, 1)); - EXPECT_TRUE(agent_host.get()); - CheckWorkerState(3, 1, WorkerState::WORKER_UNINSPECTED); - agent_host->PauseForDebugOnStart(); - CheckWorkerState(3, 1, WorkerState::WORKER_PAUSED_FOR_DEBUG_ON_START); - EXPECT_FALSE(agent_host->IsReadyForInspection()); - manager_->WorkerReadyForInspection(3, 1); - CheckWorkerState(3, 1, WorkerState::WORKER_READY_FOR_DEBUG_ON_START); - client_host->InspectAgentHost(agent_host.get()); - CheckWorkerState(3, 1, WorkerState::WORKER_INSPECTED); - client_host->InspectAgentHost(nullptr); - agent_host = nullptr; - CheckWorkerState(3, 1, WorkerState::WORKER_UNINSPECTED); - manager_->WorkerDestroyed(3, 1); - CheckWorkerNotExist(3, 1); - CheckWorkerCount(0); -} - -} // namespace content
diff --git a/content/browser/shared_worker/shared_worker_host.cc b/content/browser/shared_worker/shared_worker_host.cc index 6b09861..5d0f4580 100644 --- a/content/browser/shared_worker/shared_worker_host.cc +++ b/content/browser/shared_worker/shared_worker_host.cc
@@ -70,10 +70,8 @@ SharedWorkerHost::~SharedWorkerHost() { UMA_HISTOGRAM_LONG_TIMES("SharedWorker.TimeToDeleted", base::TimeTicks::Now() - creation_time_); - if (!closed_ && !termination_message_sent_) { - SharedWorkerDevToolsManager::GetInstance()->WorkerDestroyed(process_id_, - route_id_); - } + if (!closed_ && !termination_message_sent_) + SharedWorkerDevToolsManager::GetInstance()->WorkerDestroyed(this); } void SharedWorkerHost::Start(mojom::SharedWorkerFactoryPtr factory, @@ -127,11 +125,12 @@ } void SharedWorkerHost::TerminateWorker() { + // This can be called twice in tests while cleaning up all the workers. + if (termination_message_sent_) + return; termination_message_sent_ = true; - if (!closed_) { - SharedWorkerDevToolsManager::GetInstance()->WorkerDestroyed(process_id_, - route_id_); - } + if (!closed_) + SharedWorkerDevToolsManager::GetInstance()->WorkerDestroyed(this); worker_->Terminate(); // Now, we wait to observe OnWorkerConnectionLost. } @@ -164,15 +163,13 @@ // being sent to the worker (messages can still be sent from the worker, // for exception reporting, etc). closed_ = true; - if (!termination_message_sent_) { - SharedWorkerDevToolsManager::GetInstance()->WorkerDestroyed(process_id_, - route_id_); - } + if (!termination_message_sent_) + SharedWorkerDevToolsManager::GetInstance()->WorkerDestroyed(this); } void SharedWorkerHost::OnReadyForInspection() { - SharedWorkerDevToolsManager::GetInstance()->WorkerReadyForInspection( - process_id_, route_id_); + if (!closed_ && !termination_message_sent_) + SharedWorkerDevToolsManager::GetInstance()->WorkerReadyForInspection(this); } void SharedWorkerHost::OnScriptLoaded() {
diff --git a/content/browser/shared_worker/shared_worker_service_impl.cc b/content/browser/shared_worker/shared_worker_service_impl.cc index ecf29e78..5ed5c0b 100644 --- a/content/browser/shared_worker/shared_worker_service_impl.cc +++ b/content/browser/shared_worker/shared_worker_service_impl.cc
@@ -87,15 +87,6 @@ return false; } -bool SharedWorkerServiceImpl::TerminateWorkerById(int process_id, - int route_id) { - SharedWorkerHost* host = FindSharedWorkerHost(process_id, route_id); - if (!host || !host->instance()) - return false; - host->TerminateWorker(); - return true; -} - void SharedWorkerServiceImpl::TerminateAllWorkersForTesting( base::OnceClosure callback) { DCHECK_CURRENTLY_ON(BrowserThread::UI); @@ -210,13 +201,12 @@ // Dev Tools will need to be modified to use something else as an identifier. int worker_route_id = process_host->GetNextRoutingID(); - bool pause_on_start = - SharedWorkerDevToolsManager::GetInstance()->WorkerCreated( - worker_process_id, worker_route_id, *instance); - auto host = std::make_unique<SharedWorkerHost>( std::move(instance), worker_process_id, worker_route_id); + bool pause_on_start = + SharedWorkerDevToolsManager::GetInstance()->WorkerCreated(host.get()); + // Get the factory used to instantiate the new shared worker instance in // the target process. mojom::SharedWorkerFactoryPtr factory;
diff --git a/content/browser/shared_worker/shared_worker_service_impl.h b/content/browser/shared_worker/shared_worker_service_impl.h index ce7d66db..e634b61 100644 --- a/content/browser/shared_worker/shared_worker_service_impl.h +++ b/content/browser/shared_worker/shared_worker_service_impl.h
@@ -40,8 +40,6 @@ StoragePartition* storage_partition, ResourceContext* resource_context) override; - // Terminates the given worker. Returns true if the process was found. - bool TerminateWorkerById(int process_id, int route_id); void TerminateAllWorkersForTesting(base::OnceClosure callback); // Creates the worker if necessary or connects to an already existing worker.
diff --git a/content/test/BUILD.gn b/content/test/BUILD.gn index 6a6508e..0ac6102 100644 --- a/content/test/BUILD.gn +++ b/content/test/BUILD.gn
@@ -1203,7 +1203,6 @@ "../browser/devtools/devtools_http_handler_unittest.cc", "../browser/devtools/devtools_manager_unittest.cc", "../browser/devtools/protocol/tracing_handler_unittest.cc", - "../browser/devtools/shared_worker_devtools_manager_unittest.cc", "../browser/dom_storage/dom_storage_area_unittest.cc", "../browser/dom_storage/dom_storage_context_impl_unittest.cc", "../browser/dom_storage/dom_storage_database_unittest.cc",
diff --git a/third_party/WebKit/LayoutTests/FlagExpectations/root-layer-scrolls b/third_party/WebKit/LayoutTests/FlagExpectations/root-layer-scrolls index 88ff6ab1..a2bec00f 100644 --- a/third_party/WebKit/LayoutTests/FlagExpectations/root-layer-scrolls +++ b/third_party/WebKit/LayoutTests/FlagExpectations/root-layer-scrolls
@@ -3,7 +3,6 @@ crbug.com/417782 compositing/squashing/squash-above-fixed-1.html [ Failure ] crbug.com/417782 compositing/squashing/squash-above-fixed-3.html [ Failure ] crbug.com/417782 compositing/visibility/visibility-image-layers-dynamic.html [ Failure ] -crbug.com/417782 fast/dom/Window/window-xy-properties.html [ Failure ] crbug.com/417782 [ Linux Mac ] fast/dom/horizontal-scrollbar-when-dir-change.html [ Failure ] crbug.com/417782 fast/dom/rtl-scroll-to-leftmost-and-resize.html [ Failure ] crbug.com/417782 fast/dom/scroll-reveal-left-overflow.html [ Failure ]
diff --git a/third_party/WebKit/Source/core/events/MouseEvent.cpp b/third_party/WebKit/Source/core/events/MouseEvent.cpp index 69bfc27..38555b1c 100644 --- a/third_party/WebKit/Source/core/events/MouseEvent.cpp +++ b/third_party/WebKit/Source/core/events/MouseEvent.cpp
@@ -542,6 +542,9 @@ n = n->parentNode(); if (n) { + if (LocalFrameView* view = n->GetLayoutObject()->View()->GetFrameView()) + layer_location_ = view->DocumentToAbsolute(page_location_); + // FIXME: This logic is a wrong implementation of convertToLayerCoords. for (PaintLayer* layer = n->GetLayoutObject()->EnclosingLayer(); layer; layer = layer->Parent()) {