Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1724733002:
Show a warning bubble when the IME extension is activated. (Closed)
|
Created:
4 years, 10 months ago by Azure Wei Modified:
4 years, 8 months ago CC:
xiangye_chromium.org, chromium-reviews, extensions-reviews_chromium.org, yusukes+watch_chromium.org, tfarina, shuchen+watch_chromium.org, nona+watch_chromium.org, chromium-apps-reviews_chromium.org, James Su, hcarmona Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionShow a warning bubble when the IME extension is activated.
This cl implements showing a warning bubble when an IME extension is calling chrome.input.ime.activate() API, according to the API proposal: https://goo.gl/IeN7xE.
The input.ime.activate() API will not respond util user finishes interacting with the bubble. Only when user presses the 'OK' button, the extension call be successfully activated and get an input.ime.onActivate() event.
If user checks the 'Never show this again.' check box when clicking 'OK' button, the extension could be directly activated next time if the API is called from a user action like clicking buttons.
This cl adds the following new classes:
ImeWarningBubble: The interface or the IME warning bubble.
ImeWarningBubbleView: The implementation for the IME warning bubble. Provides warning information to the user upon the activation of an IME extension.
BUG=517773
TEST=BrowserTest.ImeActivatedBubbleBrowserTest
Committed: https://crrev.com/29221c5b81d089742ff6a4e92a35bbcf8fa98b8d
Cr-Commit-Position: refs/heads/master@{#383916}
Patch Set 1 #Patch Set 2 : #
Total comments: 12
Patch Set 3 : Fix layering problem and add test. #Patch Set 4 : Add platform "linux" and "win" for input.ime.onActivate. #
Total comments: 88
Patch Set 5 : Addressed Devlin and sky@'s comments. #Patch Set 6 : Rename test file. #
Total comments: 66
Patch Set 7 : Use BubbleDialogDelegateView. #Patch Set 8 : #
Total comments: 14
Patch Set 9 : Addressed Devlin's comments. #
Total comments: 30
Patch Set 10 : Abandon ImeWarningBubbleObserver #
Total comments: 8
Patch Set 11 : Add enum class PermissionStatus. #
Total comments: 24
Patch Set 12 : #Patch Set 13 : #
Total comments: 18
Patch Set 14 : Addressed sky@'s comments. #
Total comments: 2
Patch Set 15 : Add GetToolbarView() in BrowserWindow. #Patch Set 16 : Add ShowImeWarningBubble() in BrowserWindow. #Patch Set 17 : Update string on UI. #
Total comments: 10
Patch Set 18 : Make ImeWarningBubbleView::ShowBubble() static. #Patch Set 19 : #
Total comments: 42
Patch Set 20 : #Patch Set 21 : Update BrowserWindowCocoa on Mac. #
Total comments: 30
Patch Set 22 : Addressed Devlin's comments. #Patch Set 23 : #
Total comments: 4
Patch Set 24 : Fix patch conflict. #Messages
Total messages: 75 (19 generated)
Description was changed from ========== Show a warning bubble when the IME extension is activated. BUG=517773 TEST=None ========== to ========== Show a warning bubble when the IME extension is activated. This cl implements showing a warning bubble when an IME extension is calling chrome.input.ime.activate() API. By default, the input.ime.activate() will be failed to activate the extension, unless the extension is activated before and the user chose to never show the bubble. If the user pressed the 'OK' button on the bubble, the IME extension will be activated and the chrome.input.ime.onActivate() event will be triggered. BUG=517773 TEST=None ==========
azurewei@chromium.org changed reviewers: + rdevlin.cronin@chromium.org, shuchen@chromium.org
Description was changed from ========== Show a warning bubble when the IME extension is activated. This cl implements showing a warning bubble when an IME extension is calling chrome.input.ime.activate() API. By default, the input.ime.activate() will be failed to activate the extension, unless the extension is activated before and the user chose to never show the bubble. If the user pressed the 'OK' button on the bubble, the IME extension will be activated and the chrome.input.ime.onActivate() event will be triggered. BUG=517773 TEST=None ========== to ========== Show a warning bubble when the IME extension is activated. This cl implements showing a warning bubble when an IME extension is calling chrome.input.ime.activate() API. By default, the input.ime.activate() will be failed to activate the extension, unless the extension has been activated before and the user chose to never show the bubble. If the user pressed the 'OK' button on the bubble, the IME extension will be activated and the chrome.input.ime.onActivate() event will be triggered. BUG=517773 TEST=None ==========
Shu and Devlin, can you please help review this cl? Thank you!
I skimmed through most of this, since it looks like there's a few big layering problems that need to be addressed before a more in-depth review. Also, this will almost certainly need tests. +cc hcarmona@ - Hector, are we trying to have all new bubbles use the bubble manager framework? https://codereview.chromium.org/1724733002/diff/20001/chrome/app/chromium_str... File chrome/app/chromium_strings.grd (right): https://codereview.chromium.org/1724733002/diff/20001/chrome/app/chromium_str... chrome/app/chromium_strings.grd:1190: <message name="IDS_IME_API_ACTIVATED_HEADING" desc="First line in the content area of the IME API activated bubble. Instructs that the current IME extension will be activated."> Does this need to go in chromium_strings? https://codereview.chromium.org/1724733002/diff/20001/chrome/app/chromium_str... chrome/app/chromium_strings.grd:1191: <ph name="EXTENSION_NAME">$1<ex>Gmail Checker</ex></ph> extension may be able to collect all the text you type, including personal data like passwords and credit card numbers. Use this extension? This doesn't sound quite right to me. I think it's missing something about what the extension is trying to do (open an IME window) and why (at a high level). Otherwise, the user could be quite confused. Can you run this string by some UI folks? https://codereview.chromium.org/1724733002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h (right): https://codereview.chromium.org/1724733002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h:14: #include "ui/views/controls/button/button.h" this code cannot depend on views. https://codereview.chromium.org/1724733002/diff/20001/chrome/browser/ui/ime/i... File chrome/browser/ui/ime/ime_activated_bubble.h (right): https://codereview.chromium.org/1724733002/diff/20001/chrome/browser/ui/ime/i... chrome/browser/ui/ime/ime_activated_bubble.h:19: namespace views { This code cannot depend on views. https://codereview.chromium.org/1724733002/diff/20001/chrome/browser/ui/ime/i... File chrome/browser/ui/ime/ime_native_bubble.h (right): https://codereview.chromium.org/1724733002/diff/20001/chrome/browser/ui/ime/i... chrome/browser/ui/ime/ime_native_bubble.h:8: #include "ui/views/controls/button/button.h" ditto https://codereview.chromium.org/1724733002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/ime/ime_activated_bubble_view.cc (right): https://codereview.chromium.org/1724733002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/ime/ime_activated_bubble_view.cc:103: // Hitting this DCHECK means |ShouldShow| failed. Copy-paste fail? ;)
Description was changed from ========== Show a warning bubble when the IME extension is activated. This cl implements showing a warning bubble when an IME extension is calling chrome.input.ime.activate() API. By default, the input.ime.activate() will be failed to activate the extension, unless the extension has been activated before and the user chose to never show the bubble. If the user pressed the 'OK' button on the bubble, the IME extension will be activated and the chrome.input.ime.onActivate() event will be triggered. BUG=517773 TEST=None ========== to ========== Show a warning bubble when the IME extension is activated. This cl implements showing a warning bubble when an IME extension is calling chrome.input.ime.activate() API, according to the API proposal: https://goo.gl/IeN7xE. By default, the input.ime.activate() will be failed to activate the extension, unless the extension has been activated before and the user chose to never show the bubble. If the user pressed the 'OK' button on the bubble, the IME extension will be activated and the chrome.input.ime.onActivate() event will be triggered. This cl adds the following new classes: ImeActivatedBubble: implements the IME activated bubble. ImeNativeBubble: The interface to bridge the interactions between ImeActivatedBubble and ImeActivatedBubbleView. ImeActivatedBubbleView: Provides feedback to the user upon activation of an IME extension. ImeBubbleObserver: Interface for observing changes on the bubble view. BUG=517773 TEST=BrowserTest.ImeActivatedBubbleBrowserTest ==========
azurewei@chromium.org changed reviewers: + sky@chromium.org
On 2016/02/23 17:39:37, Devlin (Slow until 2-26) wrote: > I skimmed through most of this, since it looks like there's a few big layering > problems that need to be addressed before a more in-depth review. > > Also, this will almost certainly need tests. The layering problem was fixed. Files under chrome/browser/ui/* have not included files under ui/views/* any more. ImeActivatedBubbleBrowserTest.* were added in browser_tests. Can you please take a look? Thanks https://codereview.chromium.org/1724733002/diff/20001/chrome/app/chromium_str... File chrome/app/chromium_strings.grd (right): https://codereview.chromium.org/1724733002/diff/20001/chrome/app/chromium_str... chrome/app/chromium_strings.grd:1190: <message name="IDS_IME_API_ACTIVATED_HEADING" desc="First line in the content area of the IME API activated bubble. Instructs that the current IME extension will be activated."> On 2016/02/23 17:39:36, Devlin (Slow until 2-26) wrote: > Does this need to go in chromium_strings? I've moved this to app/generated_resources.grd, will that be better? https://codereview.chromium.org/1724733002/diff/20001/chrome/app/chromium_str... chrome/app/chromium_strings.grd:1191: <ph name="EXTENSION_NAME">$1<ex>Gmail Checker</ex></ph> extension may be able to collect all the text you type, including personal data like passwords and credit card numbers. Use this extension? On 2016/02/23 17:39:36, Devlin (Slow until 2-26) wrote: > This doesn't sound quite right to me. I think it's missing something about what > the extension is trying to do (open an IME window) and why (at a high level). > Otherwise, the user could be quite confused. Can you run this string by some UI > folks? Please review the updated text. And please see the API proposal mock: https://goo.gl/IeN7xE for the UI. The text on the mock was not updated yet. https://codereview.chromium.org/1724733002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h (right): https://codereview.chromium.org/1724733002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h:14: #include "ui/views/controls/button/button.h" On 2016/02/23 17:39:36, Devlin (Slow until 2-26) wrote: > this code cannot depend on views. Done. https://codereview.chromium.org/1724733002/diff/20001/chrome/browser/ui/ime/i... File chrome/browser/ui/ime/ime_activated_bubble.h (right): https://codereview.chromium.org/1724733002/diff/20001/chrome/browser/ui/ime/i... chrome/browser/ui/ime/ime_activated_bubble.h:19: namespace views { On 2016/02/23 17:39:37, Devlin (Slow until 2-26) wrote: > This code cannot depend on views. Done. https://codereview.chromium.org/1724733002/diff/20001/chrome/browser/ui/ime/i... File chrome/browser/ui/ime/ime_native_bubble.h (right): https://codereview.chromium.org/1724733002/diff/20001/chrome/browser/ui/ime/i... chrome/browser/ui/ime/ime_native_bubble.h:8: #include "ui/views/controls/button/button.h" On 2016/02/23 17:39:37, Devlin (Slow until 2-26) wrote: > ditto Done. https://codereview.chromium.org/1724733002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/ime/ime_activated_bubble_view.cc (right): https://codereview.chromium.org/1724733002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/ime/ime_activated_bubble_view.cc:103: // Hitting this DCHECK means |ShouldShow| failed. On 2016/02/23 17:39:37, Devlin (Slow until 2-26) wrote: > Copy-paste fail? ;) Deleted.
+ sky@ for owner of chrome/test/BUILD.gn. sky@, can you please review this cl? Thanks!
You will need someone to sign off on the extensions changes. https://codereview.chromium.org/1724733002/diff/60001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1724733002/diff/60001/chrome/app/generated_re... chrome/app/generated_resources.grd:14585: + the chrome.input.ime.* APIs, which may be able to create IME window when 'an' IME https://codereview.chromium.org/1724733002/diff/60001/chrome/app/generated_re... chrome/app/generated_resources.grd:14586: + you are typing, and collect all the text you type, including personal data like passwords and credit card numbers, Use this extension? ', Use' -> '. Use' That said, I would be inclined to say 'Are you sure you want to use this extension?' https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/ime/i... File chrome/browser/ui/ime/ime_activated_bubble.h (right): https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/ime/i... chrome/browser/ui/ime/ime_activated_bubble.h:19: namespace ui { Don't put this code in the ui namespace. The ui namespace is for code in src/ui https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/ime/i... chrome/browser/ui/ime/ime_activated_bubble.h:24: class ImeActivatedBubble { Document ownership. Also, I recommend naming this class ImeWarningBubble, that better describes what this class is for. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/ime/i... chrome/browser/ui/ime/ime_activated_bubble.h:35: std::string extension_id() { return extension_id_; } const std::string& const https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/ime/i... chrome/browser/ui/ime/ime_activated_bubble.h:41: ~ImeActivatedBubble(); You have declared this but not defined it. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/ime/i... chrome/browser/ui/ime/ime_activated_bubble.h:51: std::string extension_id_; const https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/ime/i... chrome/browser/ui/ime/ime_activated_bubble.h:52: }; DISALLOW... https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/ime/i... File chrome/browser/ui/ime/ime_bubble_observer.h (right): https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/ime/i... chrome/browser/ui/ime/ime_bubble_observer.h:10: namespace ui { Similar comment about namespace. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/ime/i... chrome/browser/ui/ime/ime_bubble_observer.h:13: class ImeBubbleObserver { ImeActivatedBubbleObserver. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/ime/i... chrome/browser/ui/ime/ime_bubble_observer.h:29: private: You don't need this for classes with pure virtual functions. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/ime/i... File chrome/browser/ui/ime/ime_native_bubble.h (right): https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/ime/i... chrome/browser/ui/ime/ime_native_bubble.h:22: virtual void PressButton(bool ok, bool checked) = 0; It's not clear what ok or checked means. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/ime/ime_activated_bubble_view.cc (right): https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/ime/ime_activated_bubble_view.cc:45: views::Button* CreateButton(const views::ButtonListener* listener, Why make this take a const* listener just to cast it away below? https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/ime/ime_activated_bubble_view.cc:47: bool blue = false) { don't use default args. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/ime/ime_activated_bubble_view.cc:49: views::BlueButton* button = new views::BlueButton( The only thing different between the two branches is the construction. The following two calls are the same. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/ime/ime_activated_bubble_view.cc:54: } else { nit: no else after return. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/ime/ime_activated_bubble_view.cc:98: DCHECK(!container->animating()); How do you know the container won't be animating? https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/ime/ime_activated_bubble_view.cc:257: Initialize(); Don't you want the bubble tied to the life of the browser? By which I mean set_parent_window? https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/ime/ime_activated_bubble_view.h (right): https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/ime/ime_activated_bubble_view.h:23: namespace ui { Similar comment about namespace. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/ime/ime_activated_bubble_view.h:26: class ImeActivatedBubbleView : public views::BubbleDelegateView, Wait for Evan to land https://codereview.chromium.org/1717453003/ and then make this use a BubbleDialogDelegateView. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/ime/ime_activated_bubble_view.h:31: enum AnchorPosition { enum class
https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc (right): https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:133: bubble_ = nullptr; Given this object is being deleted, is there a reason to set |bubble_|? https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:179: PrefService* user_prefs = profile() ? profile()->GetPrefs() : nullptr; When can profile() be null? https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:181: scoped_ptr<base::DictionaryValue> copy(new base::DictionaryValue()); If we stay with this, use a DictionaryPrefUpdate. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:186: copy->SetBoolean(bubble_->extension_id(), true); Why does this need to be a dictionary? Couldn't it be a list, with presence in the list indicating whether or not we should show the bubble? https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:187: user_prefs->Set(pref_names::kImeActivatedWarningNeverShow, *copy); When is this pref cleaned up? Overall, I think it might be better to have this be an extension pref rather than a top-level profile pref. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:191: bubble_->Close(); Move common code below the if block, i.e.: if (ok_button_pressed) { <...> SetActiveEngine(); } bubble_->Close(); bubble_ = nullptr; https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:205: bubble_->Close(); Maybe this should go in the bubble code? https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:209: void InputImeEventRouter::SetBubble(ui::ImeActivatedBubble* bubble) { Setters can go in the .h: set_bubble(Bubble* bubble) { bubble_ = bubble; } https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:217: Browser* browser = chrome::FindBrowserWithProfile(profile); Are we sure this is the right browser? https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:219: if (!browser || !event_router) Can these happen? https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:222: PrefService* user_prefs = profile ? profile->GetPrefs() : nullptr; When can profile be null? https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:247: new ui::ImeActivatedBubble(extension(), browser, event_router); Document ownership. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:250: return RespondNow(Error(kErrorUserPermission)); Is this really an error? Shouldn't we only throw an error if the user denies access? https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/ime/i... File chrome/browser/ui/ime/ime_activated_bubble.cc (right): https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/ime/i... chrome/browser/ui/ime/ime_activated_bubble.cc:9: #include "ui/views/controls/button/button.h" ? https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/ime/i... File chrome/browser/ui/ime/ime_activated_bubble.h (right): https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/ime/i... chrome/browser/ui/ime/ime_activated_bubble.h:24: class ImeActivatedBubble { Do we really need this class and ImeNativeBubble when the ImeNativeBubble is then subclassed into the platform-specific implementation? Couldn't we just have the Views impl inherit from this? https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/ime/i... File chrome/browser/ui/ime/ime_bubble_observer.h (right): https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/ime/i... chrome/browser/ui/ime/ime_bubble_observer.h:24: virtual void BubbleButtonPressed(bool ok, bool never_show_checked) = 0; I'd prefer a name other than "ok" for this variable. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/ime/ime_activated_bubble_view.cc (right): https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/ime/ime_activated_bubble_view.cc:116: location_bar_view->SetPreviewEnabledPageAction(page_action, true); Page action previews are a pain. I think it's probably fine to just anchor to the app menu for page action extensions. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/ime/ime_activated_bubble_view.cc:121: case ImeActivatedBubbleView::ANCHOR_OMNIBOX: { Why would we anchor to the omnibox? https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/ime/ime_activated_bubble_view.cc:123: reference_view = location_bar_view; Why not just assign reference view directly on line 122? https://codereview.chromium.org/1724733002/diff/60001/chrome/common/extension... File chrome/common/extensions/api/input_ime.json (right): https://codereview.chromium.org/1724733002/diff/60001/chrome/common/extension... chrome/common/extensions/api/input_ime.json:638: "platforms": ["chromeos", "linux", "chromeos"], nit: just one chromeos :)
https://codereview.chromium.org/1724733002/diff/60001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1724733002/diff/60001/chrome/app/generated_re... chrome/app/generated_resources.grd:14585: + the chrome.input.ime.* APIs, which may be able to create IME window when On 2016/02/24 18:05:49, sky wrote: > 'an' IME Done. https://codereview.chromium.org/1724733002/diff/60001/chrome/app/generated_re... chrome/app/generated_resources.grd:14586: + you are typing, and collect all the text you type, including personal data like passwords and credit card numbers, Use this extension? On 2016/02/24 18:05:48, sky wrote: > ', Use' -> '. Use' > That said, I would be inclined to say 'Are you sure you want to use this > extension?' Done. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc (right): https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:133: bubble_ = nullptr; On 2016/02/24 23:48:28, Devlin (Slow until 2-26) wrote: > Given this object is being deleted, is there a reason to set |bubble_|? This code has no use. Deleted. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:179: PrefService* user_prefs = profile() ? profile()->GetPrefs() : nullptr; On 2016/02/24 23:48:29, Devlin (Slow until 2-26) wrote: > When can profile() be null? Profile could be nullptr in tests. While, it cannot be null here since the InputImeEventRouter is created. Deleted this check. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:181: scoped_ptr<base::DictionaryValue> copy(new base::DictionaryValue()); On 2016/02/24 23:48:28, Devlin (Slow until 2-26) wrote: > If we stay with this, use a DictionaryPrefUpdate. Delete this and used ExtensionPrefs. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:186: copy->SetBoolean(bubble_->extension_id(), true); On 2016/02/24 23:48:29, Devlin (Slow until 2-26) wrote: > Why does this need to be a dictionary? Couldn't it be a list, with presence in > the list indicating whether or not we should show the bubble? Done. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:187: user_prefs->Set(pref_names::kImeActivatedWarningNeverShow, *copy); On 2016/02/24 23:48:28, Devlin (Slow until 2-26) wrote: > When is this pref cleaned up? > > Overall, I think it might be better to have this be an extension pref rather > than a top-level profile pref. The pref will be cleaned up when the extension is unloaded or uninstalled. Added the cleaning-up codes in OnExtensionUnloaded(). Replaced user_prefs with ExtensionPrefs. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:191: bubble_->Close(); On 2016/02/24 23:48:29, Devlin (Slow until 2-26) wrote: > Move common code below the if block, i.e.: > if (ok_button_pressed) { > <...> > SetActiveEngine(); > } > bubble_->Close(); > bubble_ = nullptr; Done. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:205: bubble_->Close(); On 2016/02/24 23:48:28, Devlin (Slow until 2-26) wrote: > Maybe this should go in the bubble code? Done. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:209: void InputImeEventRouter::SetBubble(ui::ImeActivatedBubble* bubble) { On 2016/02/24 23:48:29, Devlin (Slow until 2-26) wrote: > Setters can go in the .h: > set_bubble(Bubble* bubble) { bubble_ = bubble; } Done. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:217: Browser* browser = chrome::FindBrowserWithProfile(profile); On 2016/02/24 23:48:29, Devlin (Slow until 2-26) wrote: > Are we sure this is the right browser? I changed this with function FindLastActiveWithProfile(), will this be better than FindBrowserWithProfile()? https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:219: if (!browser || !event_router) On 2016/02/24 23:48:28, Devlin (Slow until 2-26) wrote: > Can these happen? event_router could be null in tests (See issue 583365 & 530804), and document find_browser.h seems also may return null, so I think it's better to just return here. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:222: PrefService* user_prefs = profile ? profile->GetPrefs() : nullptr; On 2016/02/24 23:48:28, Devlin (Slow until 2-26) wrote: > When can profile be null? profile could be null in tests, see issue 583365 & 530804. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:247: new ui::ImeActivatedBubble(extension(), browser, event_router); On 2016/02/24 23:48:29, Devlin (Slow until 2-26) wrote: > Document ownership. Done. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:250: return RespondNow(Error(kErrorUserPermission)); On 2016/02/24 23:48:28, Devlin (Slow until 2-26) wrote: > Is this really an error? Shouldn't we only throw an error if the user denies > access? The active() API will successes only this extension has been activated before and user chose never to show the warning bubble again. Otherwise, we cannot get the the results of function since the bubble button pressing event is async. On the other hand, the extension will not get any event or error if user clicks the 'CANCEL' button on the bubble. So, I think it's more clear to give an error to the extension, so it can rely on listening the onActivate() to active itself. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/ime/i... File chrome/browser/ui/ime/ime_activated_bubble.cc (right): https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/ime/i... chrome/browser/ui/ime/ime_activated_bubble.cc:9: #include "ui/views/controls/button/button.h" On 2016/02/24 23:48:29, Devlin (Slow until 2-26) wrote: > ? Deleted. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/ime/i... File chrome/browser/ui/ime/ime_activated_bubble.h (right): https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/ime/i... chrome/browser/ui/ime/ime_activated_bubble.h:19: namespace ui { On 2016/02/24 18:05:49, sky wrote: > Don't put this code in the ui namespace. The ui namespace is for code in src/ui Done. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/ime/i... chrome/browser/ui/ime/ime_activated_bubble.h:24: class ImeActivatedBubble { On 2016/02/24 23:48:29, Devlin (Slow until 2-26) wrote: > Do we really need this class and ImeNativeBubble when the ImeNativeBubble is > then subclassed into the platform-specific implementation? Couldn't we just > have the Views impl inherit from this? Combined the class ImeNativeBubble to ImeWarningBubble. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/ime/i... chrome/browser/ui/ime/ime_activated_bubble.h:24: class ImeActivatedBubble { On 2016/02/24 18:05:49, sky wrote: > Document ownership. > Also, I recommend naming this class ImeWarningBubble, that better describes what > this class is for. Done. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/ime/i... chrome/browser/ui/ime/ime_activated_bubble.h:35: std::string extension_id() { return extension_id_; } On 2016/02/24 18:05:49, sky wrote: > const std::string& const Done. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/ime/i... chrome/browser/ui/ime/ime_activated_bubble.h:41: ~ImeActivatedBubble(); On 2016/02/24 18:05:49, sky wrote: > You have declared this but not defined it. Done. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/ime/i... chrome/browser/ui/ime/ime_activated_bubble.h:51: std::string extension_id_; On 2016/02/24 18:05:49, sky wrote: > const Done. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/ime/i... chrome/browser/ui/ime/ime_activated_bubble.h:52: }; On 2016/02/24 18:05:49, sky wrote: > DISALLOW... Done. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/ime/i... File chrome/browser/ui/ime/ime_bubble_observer.h (right): https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/ime/i... chrome/browser/ui/ime/ime_bubble_observer.h:10: namespace ui { On 2016/02/24 18:05:49, sky wrote: > Similar comment about namespace. Done. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/ime/i... chrome/browser/ui/ime/ime_bubble_observer.h:13: class ImeBubbleObserver { On 2016/02/24 18:05:49, sky wrote: > ImeActivatedBubbleObserver. Done. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/ime/i... chrome/browser/ui/ime/ime_bubble_observer.h:24: virtual void BubbleButtonPressed(bool ok, bool never_show_checked) = 0; On 2016/02/24 23:48:29, Devlin (Slow until 2-26) wrote: > I'd prefer a name other than "ok" for this variable. Done. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/ime/i... chrome/browser/ui/ime/ime_bubble_observer.h:29: private: On 2016/02/24 18:05:49, sky wrote: > You don't need this for classes with pure virtual functions. Done. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/ime/i... File chrome/browser/ui/ime/ime_native_bubble.h (right): https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/ime/i... chrome/browser/ui/ime/ime_native_bubble.h:22: virtual void PressButton(bool ok, bool checked) = 0; On 2016/02/24 18:05:49, sky wrote: > It's not clear what ok or checked means. Updated the variable naming. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/ime/ime_activated_bubble_view.cc (right): https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/ime/ime_activated_bubble_view.cc:45: views::Button* CreateButton(const views::ButtonListener* listener, On 2016/02/24 18:05:49, sky wrote: > Why make this take a const* listener just to cast it away below? Yeah, this code is silly. Removed the 'const'. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/ime/ime_activated_bubble_view.cc:47: bool blue = false) { On 2016/02/24 18:05:49, sky wrote: > don't use default args. Done. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/ime/ime_activated_bubble_view.cc:49: views::BlueButton* button = new views::BlueButton( On 2016/02/24 18:05:49, sky wrote: > The only thing different between the two branches is the construction. The > following two calls are the same. Update. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/ime/ime_activated_bubble_view.cc:54: } else { On 2016/02/24 18:05:49, sky wrote: > nit: no else after return. Done. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/ime/ime_activated_bubble_view.cc:98: DCHECK(!container->animating()); On 2016/02/24 18:05:49, sky wrote: > How do you know the container won't be animating? Deleted this check as our bubble could be shown when the container is animating for now. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/ime/ime_activated_bubble_view.cc:116: location_bar_view->SetPreviewEnabledPageAction(page_action, true); On 2016/02/24 23:48:29, Devlin (Slow until 2-26) wrote: > Page action previews are a pain. I think it's probably fine to just anchor to > the app menu for page action extensions. Done. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/ime/ime_activated_bubble_view.cc:121: case ImeActivatedBubbleView::ANCHOR_OMNIBOX: { On 2016/02/24 23:48:29, Devlin (Slow until 2-26) wrote: > Why would we anchor to the omnibox? This is for extension like this: https://developer.chrome.com/extensions/omnibox. Should I also anchor it to the app menu? https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/ime/ime_activated_bubble_view.cc:123: reference_view = location_bar_view; On 2016/02/24 23:48:29, Devlin (Slow until 2-26) wrote: > Why not just assign reference view directly on line 122? Done. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/ime/ime_activated_bubble_view.cc:257: Initialize(); On 2016/02/24 18:05:49, sky wrote: > Don't you want the bubble tied to the life of the browser? By which I mean > set_parent_window? Done. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/ime/ime_activated_bubble_view.h (right): https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/ime/ime_activated_bubble_view.h:23: namespace ui { On 2016/02/24 18:05:49, sky wrote: > Similar comment about namespace. Done. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/ime/ime_activated_bubble_view.h:26: class ImeActivatedBubbleView : public views::BubbleDelegateView, On 2016/02/24 18:05:49, sky wrote: > Wait for Evan to land https://codereview.chromium.org/1717453003/ and then make > this use a BubbleDialogDelegateView. I will work on this. Thanks! https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/ime/ime_activated_bubble_view.h:31: enum AnchorPosition { On 2016/02/24 18:05:49, sky wrote: > enum class Done. https://codereview.chromium.org/1724733002/diff/60001/chrome/common/extension... File chrome/common/extensions/api/input_ime.json (right): https://codereview.chromium.org/1724733002/diff/60001/chrome/common/extension... chrome/common/extensions/api/input_ime.json:638: "platforms": ["chromeos", "linux", "chromeos"], On 2016/02/24 23:48:29, Devlin (Slow until 2-26) wrote: > nit: just one chromeos :) Fixed this typo error. Thanks!
Description was changed from ========== Show a warning bubble when the IME extension is activated. This cl implements showing a warning bubble when an IME extension is calling chrome.input.ime.activate() API, according to the API proposal: https://goo.gl/IeN7xE. By default, the input.ime.activate() will be failed to activate the extension, unless the extension has been activated before and the user chose to never show the bubble. If the user pressed the 'OK' button on the bubble, the IME extension will be activated and the chrome.input.ime.onActivate() event will be triggered. This cl adds the following new classes: ImeActivatedBubble: implements the IME activated bubble. ImeNativeBubble: The interface to bridge the interactions between ImeActivatedBubble and ImeActivatedBubbleView. ImeActivatedBubbleView: Provides feedback to the user upon activation of an IME extension. ImeBubbleObserver: Interface for observing changes on the bubble view. BUG=517773 TEST=BrowserTest.ImeActivatedBubbleBrowserTest ========== to ========== Show a warning bubble when the IME extension is activated. This cl implements showing a warning bubble when an IME extension is calling chrome.input.ime.activate() API, according to the API proposal: https://goo.gl/IeN7xE. By default, the input.ime.activate() will be failed to activate the extension, unless the extension has been activated before and the user chose to never show the bubble. If the user pressed the 'OK' button on the bubble, the IME extension will be activated and the chrome.input.ime.onActivate() event will be triggered. This cl adds the following new classes: ImeWarningBubble: The interface or the IME warning bubble. ImeWarningBubbleObserver: The interface for observing changes on the bubble view. ImeWarningBubbleView: The implementation for the IME warning bubble. Provides warning information to the user upon the activation of an IME extension. BUG=517773 TEST=BrowserTest.ImeActivatedBubbleBrowserTest ==========
https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/ime/... File chrome/browser/ui/ime/ime_warning_bubble.h (right): https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/ime/... chrome/browser/ui/ime/ime_warning_bubble.h:40: virtual bool IsVisible() = 0; Move this and the next to protected and friend your test. https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/ime/... File chrome/browser/ui/ime/ime_warning_bubble_observer.h (right): https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/ime/... chrome/browser/ui/ime/ime_warning_bubble_observer.h:11: virtual ~ImeWarningBubbleObserver() {} nit: make protected. https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/ime_warning_bubble_view.cc (right): https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:49: button = new views::BlueButton(listener, text); If use_blue_button you leak the button created on 47. https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:55: views::Checkbox* CreateCheckboxButton(const views::ButtonListener* listener, Again, don't supply a const ptr just to cast it away on the following line. https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:169: // | |CANCEL| | OK | | You should get the cancel/ok button by way of being a DialogDelegate. https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/ime_warning_bubble_view.h (right): https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.h:33: ANCHOR_BROWSER_ACTION, Since you have refer to these using AnchorPosition:: the ANCHOR_ prefix becomes redundant. Remove it.
https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc (right): https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:217: Browser* browser = chrome::FindBrowserWithProfile(profile); On 2016/02/25 09:32:12, Azure Wei wrote: > On 2016/02/24 23:48:29, Devlin (Slow until 2-26) wrote: > > Are we sure this is the right browser? > > I changed this with function FindLastActiveWithProfile(), will this be better > than FindBrowserWithProfile()? Probably better, yes. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:222: PrefService* user_prefs = profile ? profile->GetPrefs() : nullptr; On 2016/02/25 09:32:12, Azure Wei wrote: > On 2016/02/24 23:48:28, Devlin (Slow until 2-26) wrote: > > When can profile be null? > > profile could be null in tests, see issue 583365 & 530804. Those bugs are describing crashes that seem to be occurring in production, not testing? Also, if we ever execute extension api functions without a profile, chaos ensues. If you're really worried about it here, throw in a CHECK - we should find and fix it. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:250: return RespondNow(Error(kErrorUserPermission)); On 2016/02/25 09:32:12, Azure Wei wrote: > On 2016/02/24 23:48:28, Devlin (Slow until 2-26) wrote: > > Is this really an error? Shouldn't we only throw an error if the user denies > > access? > > The active() API will successes only this extension has been activated before > and user chose never to show the warning bubble again. Otherwise, we cannot get > the the results of function since the bubble button pressing event is async. On > the other hand, the extension will not get any event or error if user clicks the > 'CANCEL' button on the bubble. > So, I think it's more clear to give an error to the extension, so it can rely on > listening the onActivate() to active itself. What's wrong with waiting for the bubble to finish? Extension functions are async, so the fact that the bubble is async isn't a problem. :) And I think it's very weird to have an error when the function could succeed. https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/ime/ime_activated_bubble_view.cc (right): https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/ime/ime_activated_bubble_view.cc:121: case ImeActivatedBubbleView::ANCHOR_OMNIBOX: { On 2016/02/25 09:32:13, Azure Wei wrote: > On 2016/02/24 23:48:29, Devlin (Slow until 2-26) wrote: > > Why would we anchor to the omnibox? > > This is for extension like this: > https://developer.chrome.com/extensions/omnibox. Should I also anchor it to the > app menu? It looks like most of this code was stolen from the extension installed bubble. The reason we point to the omnibox then is because we say "To use this extension, type the keyword here". It makes no sense to point to the omnibox in this case, and I think would cause significant confusion. I think we should either anchor to the browser action or the app menu if there is no browser action (and note that soon there will always be a browser action). https://codereview.chromium.org/1724733002/diff/100001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1724733002/diff/100001/chrome/app/generated_r... chrome/app/generated_resources.grd:14584: + <ph name="EXTENSION_NAME">$1<ex>Gmail Checker</ex></ph> extension uses chrome.input.ime API, which may be able to create an IME window when you are typing, and collect all the text you type, including personal data like passwords and credit card numbers. Are you sure you want to use this extension? This is a) still really wordy for a bubble and b) overly technical (users don't know what an API is, or, I'd guess, IME). I looked at the proposal, and it seems like no UI/UX folks ever chimed in. You'll need their input here. https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc (right): https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:43: const char kErrorNotFindActiveBrowser[] = kErrorCouldNotFindActiveBrowser https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:44: "Cannot find the active browser to show the warning bubble."; remove " to show the warning bubble." https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:123: event_router->set_bubble(nullptr); Can't we just rely on BubbleClosed() being called to reset the bubble pointer? https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:206: bubble_ = nullptr; ditto https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:214: bubble_ = nullptr; ditto https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:217: ExtensionFunction::ResponseAction InputImeActivateFunction::Run() { IIRC, we also wanted to gate this on user action, right? https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:247: event_router->set_bubble(nullptr); ditto re bubble closing https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h (right): https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h:10: #include "chrome/browser/ui/ime/ime_warning_bubble.h" Would a forward declaration work? https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h:47: // Getter and setter for the current shown warning bubble. Unnecessary comment. https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h:55: // The warning bubble that is shown on the browser. Add: ", if any." https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/ime/... File chrome/browser/ui/ime/ime_warning_bubble.h (right): https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/ime/... chrome/browser/ui/ime/ime_warning_bubble.h:22: ImeWarningBubble() {} Do we need this? https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/ime/... chrome/browser/ui/ime/ime_warning_bubble.h:36: virtual const std::string extension_id() const = 0; Can this be const &? https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/ime/... chrome/browser/ui/ime/ime_warning_bubble.h:47: virtual void PressButton(bool ok_button_pressed, bool never_show_checked) = 0; For this one, also add "ForTesting", since it should definitely only ever be used in tests. https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/ime/... File chrome/browser/ui/ime/ime_warning_bubble_observer.h (right): https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/ime/... chrome/browser/ui/ime/ime_warning_bubble_observer.h:14: virtual void BubbleClosed() = 0; nit: Prefer prefixing these with "On" https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/ime_warning_bubble_browsertest.cc (right): https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_browsertest.cc:10: namespace test { I've never seen us really use a test namespace for tests... https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_browsertest.cc:45: extension_ = ExtensionBrowserTest::LoadExtension( nit: Don't need ExtensionBrowserTest:: here. https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_browsertest.cc:67: EXPECT_FALSE(ime_warning_bubble_->IsVisible()); Wait - the comments for ImeWarningBubble::Close() say that it deletes itself after close is called. Doesn't that make this super unsafe? https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/ime_warning_bubble_view.cc (right): https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:86: BrowserActionsContainer* container = What if the container *is* animating? We'll point to the wrong place. https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:105: case AnchorPosition::ANCHOR_PAGE_ACTION: Just remove the page action and omnibox anchor options. In fact, since there will only be two, I'd recommend just checking for the browser action, and if it's not there, fallback to the app menu, and removing the anchor enum entirely. https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:123: browser_view->GetLocationBarView()->SetPreviewEnabledPageAction( Preview was never enabled. https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:157: return false; Why do we override this? https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:256: if (sender == ok_button_) { Simpler: DCHECK(sender == ok_button_ || sender == cancel_button_); bool ok_button_pressed = sender == ok_button_; FOR_EACH_OBSERVER(Observer, observers_, BubbleButtonPressed(ok_button_pressed, checked)); https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:279: if (extension_) When is extension_ null? https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/ime_warning_bubble_view.h (right): https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.h:26: // release the instance instead of the private destructor. nit: I think it's obvious that clients should call the private dtor (it's private! ;)). We could probably just say "...is self-owned and deletes itself when it is closed." https://codereview.chromium.org/1724733002/diff/100001/extensions/browser/ext... File extensions/browser/extension_prefs.cc (right): https://codereview.chromium.org/1724733002/diff/100001/extensions/browser/ext... extensions/browser/extension_prefs.cc:1781: registry->RegisterDictionaryPref(pref_names::kImeActivatedWarningNeverShow); We don't do this for prefs that are specific to a single extension. https://codereview.chromium.org/1724733002/diff/100001/extensions/browser/pre... File extensions/browser/pref_names.h (right): https://codereview.chromium.org/1724733002/diff/100001/extensions/browser/pre... extensions/browser/pref_names.h:118: extern const char kImeActivatedWarningNeverShow[]; We don't put prefs specific to a single extension here - instead, just put it in the file where it's used.
+cc xiangye@, xiangye, could you please help with the string review? Thank you!
https://codereview.chromium.org/1724733002/diff/100001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1724733002/diff/100001/chrome/app/generated_r... chrome/app/generated_resources.grd:14584: + <ph name="EXTENSION_NAME">$1<ex>Gmail Checker</ex></ph> extension uses chrome.input.ime API, which may be able to create an IME window when you are typing, and collect all the text you type, including personal data like passwords and credit card numbers. Are you sure you want to use this extension? On 2016/02/25 22:38:50, Devlin wrote: > This is a) still really wordy for a bubble and b) overly technical (users don't > know what an API is, or, I'd guess, IME). I looked at the proposal, and it > seems like no UI/UX folks ever chimed in. You'll need their input here. Asked xiangye@'s help for contacting the UI/UX reviewers. Will update this latter. https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc (right): https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:43: const char kErrorNotFindActiveBrowser[] = On 2016/02/25 22:38:50, Devlin wrote: > kErrorCouldNotFindActiveBrowser Done. https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:44: "Cannot find the active browser to show the warning bubble."; On 2016/02/25 22:38:50, Devlin wrote: > remove " to show the warning bubble." Done. https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:123: event_router->set_bubble(nullptr); On 2016/02/25 22:38:50, Devlin wrote: > Can't we just rely on BubbleClosed() being called to reset the bubble pointer? Yeah, all these 'set_bubble(nullptr)' could be done in BubbleClosed(). Thank you. https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:206: bubble_ = nullptr; On 2016/02/25 22:38:50, Devlin wrote: > ditto Done. https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:214: bubble_ = nullptr; On 2016/02/25 22:38:50, Devlin wrote: > ditto Done. https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:217: ExtensionFunction::ResponseAction InputImeActivateFunction::Run() { On 2016/02/25 22:38:50, Devlin wrote: > IIRC, we also wanted to gate this on user action, right? Sorry that forget to add the user action check. Added the ExtensionFunction::user_gesture() check in the automatically activation when user has been allowed to activate the extension before. https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:247: event_router->set_bubble(nullptr); On 2016/02/25 22:38:50, Devlin wrote: > ditto re bubble closing Done. https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h (right): https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h:10: #include "chrome/browser/ui/ime/ime_warning_bubble.h" On 2016/02/25 22:38:51, Devlin wrote: > Would a forward declaration work? Done. https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h:47: // Getter and setter for the current shown warning bubble. On 2016/02/25 22:38:50, Devlin wrote: > Unnecessary comment. Removed. https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h:55: // The warning bubble that is shown on the browser. On 2016/02/25 22:38:51, Devlin wrote: > Add: ", if any." Done. https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/ime/... File chrome/browser/ui/ime/ime_warning_bubble.h (right): https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/ime/... chrome/browser/ui/ime/ime_warning_bubble.h:22: ImeWarningBubble() {} On 2016/02/25 22:38:51, Devlin wrote: > Do we need this? Removed. https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/ime/... chrome/browser/ui/ime/ime_warning_bubble.h:36: virtual const std::string extension_id() const = 0; On 2016/02/25 22:38:51, Devlin wrote: > Can this be const &? Done. https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/ime/... chrome/browser/ui/ime/ime_warning_bubble.h:40: virtual bool IsVisible() = 0; On 2016/02/25 17:25:35, sky wrote: > Move this and the next to protected and friend your test. Done. https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/ime/... chrome/browser/ui/ime/ime_warning_bubble.h:47: virtual void PressButton(bool ok_button_pressed, bool never_show_checked) = 0; On 2016/02/25 22:38:51, Devlin wrote: > For this one, also add "ForTesting", since it should definitely only ever be > used in tests. Done. https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/ime/... File chrome/browser/ui/ime/ime_warning_bubble_observer.h (right): https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/ime/... chrome/browser/ui/ime/ime_warning_bubble_observer.h:11: virtual ~ImeWarningBubbleObserver() {} On 2016/02/25 17:25:35, sky wrote: > nit: make protected. Done. https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/ime/... chrome/browser/ui/ime/ime_warning_bubble_observer.h:14: virtual void BubbleClosed() = 0; On 2016/02/25 22:38:51, Devlin wrote: > nit: Prefer prefixing these with "On" Done. https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/ime_warning_bubble_browsertest.cc (right): https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_browsertest.cc:10: namespace test { On 2016/02/25 22:38:51, Devlin wrote: > I've never seen us really use a test namespace for tests... Removed this namespace. https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_browsertest.cc:45: extension_ = ExtensionBrowserTest::LoadExtension( On 2016/02/25 22:38:51, Devlin wrote: > nit: Don't need ExtensionBrowserTest:: here. Done. https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_browsertest.cc:67: EXPECT_FALSE(ime_warning_bubble_->IsVisible()); On 2016/02/25 22:38:51, Devlin wrote: > Wait - the comments for ImeWarningBubble::Close() say that it deletes itself > after close is called. Doesn't that make this super unsafe? Oh, IsVisible() should not be called when the bubble is closed. Thanks for pointing it out. https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/ime_warning_bubble_view.cc (right): https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:49: button = new views::BlueButton(listener, text); On 2016/02/25 17:25:35, sky wrote: > If use_blue_button you leak the button created on 47. As BubbleDialogDelegateView is used, this function has been removed. https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:55: views::Checkbox* CreateCheckboxButton(const views::ButtonListener* listener, On 2016/02/25 17:25:35, sky wrote: > Again, don't supply a const ptr just to cast it away on the following line. Done. https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:86: BrowserActionsContainer* container = On 2016/02/25 22:38:51, Devlin wrote: > What if the container *is* animating? We'll point to the wrong place. Added the animating check and used ThreadTaskRunnerHandle::PostDelayedTask() to make sure we would point to the right place. https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:105: case AnchorPosition::ANCHOR_PAGE_ACTION: On 2016/02/25 22:38:51, Devlin wrote: > Just remove the page action and omnibox anchor options. > > In fact, since there will only be two, I'd recommend just checking for the > browser action, and if it's not there, fallback to the app menu, and removing > the anchor enum entirely. Done. https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:123: browser_view->GetLocationBarView()->SetPreviewEnabledPageAction( On 2016/02/25 22:38:51, Devlin wrote: > Preview was never enabled. Removed this override. https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:157: return false; On 2016/02/25 22:38:51, Devlin wrote: > Why do we override this? Removed. https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:169: // | |CANCEL| | OK | | On 2016/02/25 17:25:35, sky wrote: > You should get the cancel/ok button by way of being a DialogDelegate. Used BubbleDialogDelegateView to get the cancel/ok button. https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:256: if (sender == ok_button_) { On 2016/02/25 22:38:51, Devlin wrote: > Simpler: > DCHECK(sender == ok_button_ || sender == cancel_button_); > bool ok_button_pressed = sender == ok_button_; > FOR_EACH_OBSERVER(Observer, observers_, BubbleButtonPressed(ok_button_pressed, > checked)); Done. https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:279: if (extension_) On 2016/02/25 22:38:51, Devlin wrote: > When is extension_ null? The initial idea was to allow the tests to new an ImeWarningBubbleView with null extension. While it's more convenient to load an extension in tests. Updated this with DCHECK(!extension_) to make sure the extension_ is never null. https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/ime_warning_bubble_view.h (right): https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.h:26: // release the instance instead of the private destructor. On 2016/02/25 22:38:51, Devlin wrote: > nit: I think it's obvious that clients should call the private dtor (it's > private! ;)). We could probably just say "...is self-owned and deletes itself > when it is closed." Done. https://codereview.chromium.org/1724733002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.h:33: ANCHOR_BROWSER_ACTION, On 2016/02/25 17:25:35, sky wrote: > Since you have refer to these using AnchorPosition:: the ANCHOR_ prefix becomes > redundant. Remove it. This enum class has been deleted. https://codereview.chromium.org/1724733002/diff/100001/extensions/browser/ext... File extensions/browser/extension_prefs.cc (right): https://codereview.chromium.org/1724733002/diff/100001/extensions/browser/ext... extensions/browser/extension_prefs.cc:1781: registry->RegisterDictionaryPref(pref_names::kImeActivatedWarningNeverShow); On 2016/02/25 22:38:51, Devlin wrote: > We don't do this for prefs that are specific to a single extension. Done. https://codereview.chromium.org/1724733002/diff/100001/extensions/browser/pre... File extensions/browser/pref_names.h (right): https://codereview.chromium.org/1724733002/diff/100001/extensions/browser/pre... extensions/browser/pref_names.h:118: extern const char kImeActivatedWarningNeverShow[]; On 2016/02/25 22:38:51, Devlin wrote: > We don't put prefs specific to a single extension here - instead, just put it in > the file where it's used. Done.
Sorry, I wasn't able to get to this today. :( I'll do my best to do it tomorrow!
https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc (right): https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:250: return RespondNow(Error(kErrorUserPermission)); On 2016/02/25 22:38:50, Devlin wrote: > On 2016/02/25 09:32:12, Azure Wei wrote: > > On 2016/02/24 23:48:28, Devlin (Slow until 2-26) wrote: > > > Is this really an error? Shouldn't we only throw an error if the user > denies > > > access? > > > > The active() API will successes only this extension has been activated before > > and user chose never to show the warning bubble again. Otherwise, we cannot > get > > the the results of function since the bubble button pressing event is async. > On > > the other hand, the extension will not get any event or error if user clicks > the > > 'CANCEL' button on the bubble. > > So, I think it's more clear to give an error to the extension, so it can rely > on > > listening the onActivate() to active itself. > > What's wrong with waiting for the bubble to finish? Extension functions are > async, so the fact that the bubble is async isn't a problem. :) And I think > it's very weird to have an error when the function could succeed. ^^ https://codereview.chromium.org/1724733002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc (right): https://codereview.chromium.org/1724733002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:137: // Cleans up the extension preferences if the extension is unloaded. OnExtensionUnloaded probably isn't the right place for this. Is there a reason to manually remove the prefs rather than just letting them be deleted if the extension is uninstalled? https://codereview.chromium.org/1724733002/diff/140001/chrome/browser/ui/ime/... File chrome/browser/ui/ime/ime_warning_bubble_observer.h (right): https://codereview.chromium.org/1724733002/diff/140001/chrome/browser/ui/ime/... chrome/browser/ui/ime/ime_warning_bubble_observer.h:15: virtual void OnClosed(ImeWarningBubble* bubble) = 0; Let's make these a bit more descriptive - OnImeWarningBubbleClosed(). Otherwise, EventRouter::OnClosed() is very strange sounding. https://codereview.chromium.org/1724733002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/ime_warning_bubble_browsertest.cc (right): https://codereview.chromium.org/1724733002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_browsertest.cc:74: base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(kWaitMs)); Sleeping is almost always wrong in a test, not least because it's impossible to know what a good amount of time to sleep is (e.g., what about on our Win Dr Memory tests where the timeout is several minutes because everything is so slow?) https://codereview.chromium.org/1724733002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/ime_warning_bubble_view.cc (right): https://codereview.chromium.org/1724733002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:175: base::ThreadTaskRunnerHandle::Get()->PostDelayedTask( What happens when we get past animation_wait_retries_? https://codereview.chromium.org/1724733002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:198: // Waits for the bubble showing before closing it. Why do we have to wait before we automatically close it? Wouldn't it be better to not show it at all? https://codereview.chromium.org/1724733002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/ime_warning_bubble_view.h (right): https://codereview.chromium.org/1724733002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.h:68: bool IsAnimating(); This is strangely named since it's checking the toolbar, rather than the bubble.
https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc (right): https://codereview.chromium.org/1724733002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:250: return RespondNow(Error(kErrorUserPermission)); On 2016/03/04 00:18:40, Devlin wrote: > On 2016/02/25 22:38:50, Devlin wrote: > > On 2016/02/25 09:32:12, Azure Wei wrote: > > > On 2016/02/24 23:48:28, Devlin (Slow until 2-26) wrote: > > > > Is this really an error? Shouldn't we only throw an error if the user > > denies > > > > access? > > > > > > The active() API will successes only this extension has been activated > before > > > and user chose never to show the warning bubble again. Otherwise, we cannot > > get > > > the the results of function since the bubble button pressing event is async. > > On > > > the other hand, the extension will not get any event or error if user clicks > > the > > > 'CANCEL' button on the bubble. > > > So, I think it's more clear to give an error to the extension, so it can > rely > > on > > > listening the onActivate() to active itself. > > > > What's wrong with waiting for the bubble to finish? Extension functions are > > async, so the fact that the bubble is async isn't a problem. :) And I think > > it's very weird to have an error when the function could succeed. > > ^^ Sorry for missing this comment. Updated this API whit using ResponseLater() to wait for the bubble to finish. And returned error only when the bubble is closed without user pressing the 'OK' button. https://codereview.chromium.org/1724733002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc (right): https://codereview.chromium.org/1724733002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:137: // Cleans up the extension preferences if the extension is unloaded. On 2016/03/04 00:18:40, Devlin wrote: > OnExtensionUnloaded probably isn't the right place for this. Is there a reason > to manually remove the prefs rather than just letting them be deleted if the > extension is uninstalled? No particular reasons for doing this. Just thought the extension should request for permission again if it is reloaded. It also makes sense to keep the activated information for load/unload. Removed this logic. https://codereview.chromium.org/1724733002/diff/140001/chrome/browser/ui/ime/... File chrome/browser/ui/ime/ime_warning_bubble_observer.h (right): https://codereview.chromium.org/1724733002/diff/140001/chrome/browser/ui/ime/... chrome/browser/ui/ime/ime_warning_bubble_observer.h:15: virtual void OnClosed(ImeWarningBubble* bubble) = 0; On 2016/03/04 00:18:41, Devlin wrote: > Let's make these a bit more descriptive - OnImeWarningBubbleClosed(). > Otherwise, EventRouter::OnClosed() is very strange sounding. Done. https://codereview.chromium.org/1724733002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/ime_warning_bubble_browsertest.cc (right): https://codereview.chromium.org/1724733002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_browsertest.cc:74: base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(kWaitMs)); On 2016/03/04 00:18:41, Devlin wrote: > Sleeping is almost always wrong in a test, not least because it's impossible to > know what a good amount of time to sleep is (e.g., what about on our Win Dr > Memory tests where the timeout is several minutes because everything is so > slow?) Added a new private testing function: ShowBubbleForTesting() in ImeWarningBubble. Thus we just test the easy showing situation which doesn't involve 'waiting for toolbar animation' logic. Will this be better? https://codereview.chromium.org/1724733002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/ime_warning_bubble_view.cc (right): https://codereview.chromium.org/1724733002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:175: base::ThreadTaskRunnerHandle::Get()->PostDelayedTask( On 2016/03/04 00:18:41, Devlin wrote: > What happens when we get past animation_wait_retries_? Oh, this may cause memory leak since the ImeWarningBubbleView will not be deleted. Added calling CloseBubble() here. https://codereview.chromium.org/1724733002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:198: // Waits for the bubble showing before closing it. On 2016/03/04 00:18:41, Devlin wrote: > Why do we have to wait before we automatically close it? Wouldn't it be better > to not show it at all? These codes are clumsy as I tried to make sure GetWidget()->Close() will be called to release the ImeWarningBubbleView instance. Updated this with directly calling delete this. https://codereview.chromium.org/1724733002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/ime_warning_bubble_view.h (right): https://codereview.chromium.org/1724733002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.h:68: bool IsAnimating(); On 2016/03/04 00:18:41, Devlin wrote: > This is strangely named since it's checking the toolbar, rather than the bubble. Renamed it as IsToolbarAnimating(). Is this better?
(just responding, will take another look later) https://codereview.chromium.org/1724733002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/ime_warning_bubble_browsertest.cc (right): https://codereview.chromium.org/1724733002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_browsertest.cc:74: base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(kWaitMs)); On 2016/03/04 13:09:17, Azure Wei wrote: > On 2016/03/04 00:18:41, Devlin wrote: > > Sleeping is almost always wrong in a test, not least because it's impossible > to > > know what a good amount of time to sleep is (e.g., what about on our Win Dr > > Memory tests where the timeout is several minutes because everything is so > > slow?) > > Added a new private testing function: ShowBubbleForTesting() in > ImeWarningBubble. Thus we just test the easy showing situation which doesn't > involve 'waiting for toolbar animation' logic. Will this be better? If this is only needed to account for the toolbar animating, we have ToolbarActionsBar::disable_animations_for_testing_, which would probably be better (since it lets us go through more of the normal flow).
https://codereview.chromium.org/1724733002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc (right): https://codereview.chromium.org/1724733002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:135: event_router->bubble()->CloseBubble(); What if the bubble was shown for a different extension? https://codereview.chromium.org/1724733002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:212: if (!ok_button_pressed_) { What if multiple bubbles were shown? Wouldn't ok_button_pressed_ then (potentially) have the wrong value? https://codereview.chromium.org/1724733002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:229: if (user_gesture() && I think I'd prefer to require a user gesture in all cases. Is there a reason not to? https://codereview.chromium.org/1724733002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:247: // If another warning bubble is shown, close the bubble and not activate One extension shouldn't be allowed to cause the closing of the bubble of another extension. https://codereview.chromium.org/1724733002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:257: event_router->set_bubble(bubble); nit: move this above the call to ShowBubble(). https://codereview.chromium.org/1724733002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:260: event_router->set_response_callback(callback); Why not just have this object be the bubble observer? https://codereview.chromium.org/1724733002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h (right): https://codereview.chromium.org/1724733002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h:28: typedef base::Callback<void(bool allow_to_activate)> GetUserResponseCallback; nit: prefer "using" in new code. https://codereview.chromium.org/1724733002/diff/160001/chrome/browser/ui/ime/... File chrome/browser/ui/ime/ime_warning_bubble.h (right): https://codereview.chromium.org/1724733002/diff/160001/chrome/browser/ui/ime/... chrome/browser/ui/ime/ime_warning_bubble.h:19: // The ImeWarningBubble is self-owned, it deletes itself when Close() is called. nit: CloseBubble() https://codereview.chromium.org/1724733002/diff/160001/chrome/browser/ui/ime/... File chrome/browser/ui/ime/ime_warning_bubble_observer.h (right): https://codereview.chromium.org/1724733002/diff/160001/chrome/browser/ui/ime/... chrome/browser/ui/ime/ime_warning_bubble_observer.h:15: virtual void OnImeWarningBubbleClosed(ImeWarningBubble* bubble) = 0; Do you anticipate needing more methods on the observer class? If not, could we just pass in a callback to the bubble and remove this whole class? https://codereview.chromium.org/1724733002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/ime_warning_bubble_view.cc (right): https://codereview.chromium.org/1724733002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:94: reference_view = nullptr; // fall back to app menu below. In this case, doesn't it mean we're no longer anchoring to the browser action? https://codereview.chromium.org/1724733002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:105: void ImeWarningBubbleView::OnWidgetDestroyed(views::Widget* widget) { You should probably call the BubbleDelegateView() versions of these, too, unless you're deliberately not (in which case maybe also include a comment as to why). https://codereview.chromium.org/1724733002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:112: if (!active && widget == GetWidget()) Is there a reason you're overriding the default behavior from BubbleDelegateView? https://codereview.chromium.org/1724733002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:130: views::GridLayout* layout = new views::GridLayout(this); sky@ would know best, but this strikes me as not needing a GridLayout (since you're not doing anything grid-y). https://codereview.chromium.org/1724733002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:169: if (!IsToolbarAnimating()) { Wait... IsToolbarAnimating() returns false if !anchor_to_browser_action_, but anchor_to_browser_action is only set in InitAnchorView(), which is called on line 171. Doesn't that mean that IsToolbarAnimating() is always false here? https://codereview.chromium.org/1724733002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:251: DCHECK(!extension_); ? We DCHECK(!extension_) and then dereference extension_ What am I missing?
Description was changed from ========== Show a warning bubble when the IME extension is activated. This cl implements showing a warning bubble when an IME extension is calling chrome.input.ime.activate() API, according to the API proposal: https://goo.gl/IeN7xE. By default, the input.ime.activate() will be failed to activate the extension, unless the extension has been activated before and the user chose to never show the bubble. If the user pressed the 'OK' button on the bubble, the IME extension will be activated and the chrome.input.ime.onActivate() event will be triggered. This cl adds the following new classes: ImeWarningBubble: The interface or the IME warning bubble. ImeWarningBubbleObserver: The interface for observing changes on the bubble view. ImeWarningBubbleView: The implementation for the IME warning bubble. Provides warning information to the user upon the activation of an IME extension. BUG=517773 TEST=BrowserTest.ImeActivatedBubbleBrowserTest ========== to ========== Show a warning bubble when the IME extension is activated. This cl implements showing a warning bubble when an IME extension is calling chrome.input.ime.activate() API, according to the API proposal: https://goo.gl/IeN7xE. The input.ime.activate() API will not respond util user finishes interacting with the bubble. Only when user presses the 'OK' button, the extension call be successfully activated and get an input.ime.onActivate() event. If user checks the 'Never show this again.' check box when clicking 'OK' button, the extension could be directly activated next time if the API is called from a user action like clicking buttons. This cl adds the following new classes: ImeWarningBubble: The interface or the IME warning bubble. ImeWarningBubbleView: The implementation for the IME warning bubble. Provides warning information to the user upon the activation of an IME extension. BUG=517773 TEST=BrowserTest.ImeActivatedBubbleBrowserTest ==========
https://codereview.chromium.org/1724733002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/ime_warning_bubble_browsertest.cc (right): https://codereview.chromium.org/1724733002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_browsertest.cc:74: base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(kWaitMs)); On 2016/03/04 17:23:39, Devlin wrote: > On 2016/03/04 13:09:17, Azure Wei wrote: > > On 2016/03/04 00:18:41, Devlin wrote: > > > Sleeping is almost always wrong in a test, not least because it's impossible > > to > > > know what a good amount of time to sleep is (e.g., what about on our Win Dr > > > Memory tests where the timeout is several minutes because everything is so > > > slow?) > > > > Added a new private testing function: ShowBubbleForTesting() in > > ImeWarningBubble. Thus we just test the easy showing situation which doesn't > > involve 'waiting for toolbar animation' logic. Will this be better? > > If this is only needed to account for the toolbar animating, we have > ToolbarActionsBar::disable_animations_for_testing_, which would probably be > better (since it lets us go through more of the normal flow). Done. Thank you! https://codereview.chromium.org/1724733002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc (right): https://codereview.chromium.org/1724733002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:135: event_router->bubble()->CloseBubble(); On 2016/03/08 00:01:37, Devlin wrote: > What if the bubble was shown for a different extension? bubble_ was removed from the InputImeEventRouter. https://codereview.chromium.org/1724733002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:212: if (!ok_button_pressed_) { On 2016/03/08 00:01:37, Devlin wrote: > What if multiple bubbles were shown? Wouldn't ok_button_pressed_ then > (potentially) have the wrong value? The bubble is tied to the InputImeActivateFunction of each calls. It should always get the right bubble now. https://codereview.chromium.org/1724733002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:229: if (user_gesture() && On 2016/03/08 00:01:37, Devlin wrote: > I think I'd prefer to require a user gesture in all cases. Is there a reason > not to? Done. https://codereview.chromium.org/1724733002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:247: // If another warning bubble is shown, close the bubble and not activate On 2016/03/08 00:01:37, Devlin wrote: > One extension shouldn't be allowed to cause the closing of the bubble of another > extension. Done. https://codereview.chromium.org/1724733002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:257: event_router->set_bubble(bubble); On 2016/03/08 00:01:37, Devlin wrote: > nit: move this above the call to ShowBubble(). Done. https://codereview.chromium.org/1724733002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:260: event_router->set_response_callback(callback); On 2016/03/08 00:01:37, Devlin wrote: > Why not just have this object be the bubble observer? This was because I tried to keep only one bubble at any time. With that thought, it was more easy to keep the bubble in the singleton InputImeEventRouter. Addressed your another comment, the ImeWarningObserver class was abandoned. https://codereview.chromium.org/1724733002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h (right): https://codereview.chromium.org/1724733002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h:28: typedef base::Callback<void(bool allow_to_activate)> GetUserResponseCallback; On 2016/03/08 00:01:37, Devlin wrote: > nit: prefer "using" in new code. Done. https://codereview.chromium.org/1724733002/diff/160001/chrome/browser/ui/ime/... File chrome/browser/ui/ime/ime_warning_bubble.h (right): https://codereview.chromium.org/1724733002/diff/160001/chrome/browser/ui/ime/... chrome/browser/ui/ime/ime_warning_bubble.h:19: // The ImeWarningBubble is self-owned, it deletes itself when Close() is called. On 2016/03/08 00:01:37, Devlin wrote: > nit: CloseBubble() Comments are updated. The method CloseBubble() was deleted. https://codereview.chromium.org/1724733002/diff/160001/chrome/browser/ui/ime/... File chrome/browser/ui/ime/ime_warning_bubble_observer.h (right): https://codereview.chromium.org/1724733002/diff/160001/chrome/browser/ui/ime/... chrome/browser/ui/ime/ime_warning_bubble_observer.h:15: virtual void OnImeWarningBubbleClosed(ImeWarningBubble* bubble) = 0; On 2016/03/08 00:01:37, Devlin wrote: > Do you anticipate needing more methods on the observer class? If not, could we > just pass in a callback to the bubble and remove this whole class? Yes, the whole call could be removed. Thanks for your comments. https://codereview.chromium.org/1724733002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/ime_warning_bubble_view.cc (right): https://codereview.chromium.org/1724733002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:94: reference_view = nullptr; // fall back to app menu below. On 2016/03/08 00:01:37, Devlin wrote: > In this case, doesn't it mean we're no longer anchoring to the browser action? Done. https://codereview.chromium.org/1724733002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:105: void ImeWarningBubbleView::OnWidgetDestroyed(views::Widget* widget) { On 2016/03/08 00:01:37, Devlin wrote: > You should probably call the BubbleDelegateView() versions of these, too, unless > you're deliberately not (in which case maybe also include a comment as to why). Done. https://codereview.chromium.org/1724733002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:112: if (!active && widget == GetWidget()) On 2016/03/08 00:01:37, Devlin wrote: > Is there a reason you're overriding the default behavior from > BubbleDelegateView? Oh, no need for doing this. https://codereview.chromium.org/1724733002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:130: views::GridLayout* layout = new views::GridLayout(this); On 2016/03/08 00:01:37, Devlin wrote: > sky@ would know best, but this strikes me as not needing a GridLayout (since > you're not doing anything grid-y). GridLyout is used as it seems more convenient for controlling than BoxLayout and FillLayout. sky@, should I replace this with other LayoutManager? https://codereview.chromium.org/1724733002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:169: if (!IsToolbarAnimating()) { On 2016/03/08 00:01:37, Devlin wrote: > Wait... IsToolbarAnimating() returns false if !anchor_to_browser_action_, but > anchor_to_browser_action is only set in InitAnchorView(), which is called on > line 171. Doesn't that mean that IsToolbarAnimating() is always false here? This was a wrong update from the last patch set, as I thought it would be more efficient to call InitXxx methods after the toolbar animating. Fixed this bug. https://codereview.chromium.org/1724733002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:251: DCHECK(!extension_); On 2016/03/08 00:01:37, Devlin wrote: > ? > > We DCHECK(!extension_) > and then dereference extension_ > What am I missing? Removed this method.
This looks pretty good to me, but I'd like Scott to weigh in. https://codereview.chromium.org/1724733002/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h (right): https://codereview.chromium.org/1724733002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h:13: class ImeWarningBubble; Unneeded https://codereview.chromium.org/1724733002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/ime_warning_bubble_view.cc (right): https://codereview.chromium.org/1724733002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:83: // Anchros the bubble to the browser action of the extension. typo: anchors https://codereview.chromium.org/1724733002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:103: response_callback_.Run(false, never_show_checkbox_->checked()); It's a bit weird to send whether or not the checkbox was checked if the bubble closes without the user clicking a button. I think the best solution might be an enum: PERMISSION_GRANTED, PERMISSION_GRANTED_AND_NEVER_SHOW, PERMISSION_DENIED, ABORTED But, if you don't like that, I'd probably at least default to false here. https://codereview.chromium.org/1724733002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:108: if (!active && widget == GetWidget()) We established in PatchSet 9 this wasn't needed, right?
https://codereview.chromium.org/1724733002/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h (right): https://codereview.chromium.org/1724733002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h:13: class ImeWarningBubble; On 2016/03/14 22:41:03, Devlin wrote: > Unneeded Done. https://codereview.chromium.org/1724733002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/ime_warning_bubble_view.cc (right): https://codereview.chromium.org/1724733002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:83: // Anchros the bubble to the browser action of the extension. On 2016/03/14 22:41:04, Devlin wrote: > typo: anchors Done. https://codereview.chromium.org/1724733002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:103: response_callback_.Run(false, never_show_checkbox_->checked()); On 2016/03/14 22:41:04, Devlin wrote: > It's a bit weird to send whether or not the checkbox was checked if the bubble > closes without the user clicking a button. I think the best solution might be > an enum: > PERMISSION_GRANTED, > PERMISSION_GRANTED_AND_NEVER_SHOW, > PERMISSION_DENIED, > ABORTED > > But, if you don't like that, I'd probably at least default to false here. The enum class ImeWarningBubble::PermissionStatus was added. Thanks for the comments. https://codereview.chromium.org/1724733002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:108: if (!active && widget == GetWidget()) On 2016/03/14 22:41:03, Devlin wrote: > We established in PatchSet 9 this wasn't needed, right? Done. Sorry about the mistake.
The CQ bit was checked by shuchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1724733002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1724733002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
https://codereview.chromium.org/1724733002/diff/200001/chrome/browser/ui/ime/... File chrome/browser/ui/ime/ime_warning_bubble.h (right): https://codereview.chromium.org/1724733002/diff/200001/chrome/browser/ui/ime/... chrome/browser/ui/ime/ime_warning_bubble.h:20: // The ImeWarningBubble is self-owned, it deletes itself the widget is closed the->'when the' https://codereview.chromium.org/1724733002/diff/200001/chrome/browser/ui/ime/... chrome/browser/ui/ime/ime_warning_bubble.h:21: // or fails to create the widget. AFAICT this never fails. Am I missing something? https://codereview.chromium.org/1724733002/diff/200001/chrome/browser/ui/ime/... chrome/browser/ui/ime/ime_warning_bubble.h:27: PERMISSION_GRANTED, PermissionStatus::PERMISSION_GRANTED seems a bit redundant. Did you consider nuking PERMISSION_? So that here you would have PermissionStatus::GRANTED. https://codereview.chromium.org/1724733002/diff/200001/chrome/browser/ui/ime/... chrome/browser/ui/ime/ime_warning_bubble.h:55: virtual void PressButtonForTesting(bool ok_button_pressed, PressButtonForTesting is a rather confusing name for this function. I think it's more of CompleteForTesting or CloseForTesting https://codereview.chromium.org/1724733002/diff/200001/chrome/browser/ui/ime/... chrome/browser/ui/ime/ime_warning_bubble.h:58: virtual ~ImeWarningBubble() {} destructor before other functions. https://codereview.chromium.org/1724733002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/ime_warning_bubble_browsertest.cc (right): https://codereview.chromium.org/1724733002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_browsertest.cc:63: if (status == ImeWarningBubble::PermissionStatus::PERMISSION_GRANTED || nit: {} https://codereview.chromium.org/1724733002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/ime_warning_bubble_view.cc (right): https://codereview.chromium.org/1724733002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:36: // The coloumn width of the warning bubble. column https://codereview.chromium.org/1724733002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:54: checkbox->set_listener(listener); AFAICT you don't actually need the listener. https://codereview.chromium.org/1724733002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:55: checkbox->SetHorizontalAlignment(gfx::ALIGN_LEFT); ALIGN_LEFT is the default. I think you can nuke this function and create the CheckBox directly. https://codereview.chromium.org/1724733002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:174: base::ThreadTaskRunnerHandle::Get()->PostDelayedTask( Delaying until you hope things are better inevitably causes problems. For example, because you're adding a delay it's entirely possible browser_ may be destroyed by the time your callback is run. You need a way to directly observe when the toolbar is done animating, and you need to deal with the browser_ being deleted before too. https://codereview.chromium.org/1724733002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:191: if (never_show_checkbox_->checked()) nit: {} in here.
The CQ bit was checked by azurewei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1724733002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1724733002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
https://codereview.chromium.org/1724733002/diff/200001/chrome/browser/ui/ime/... File chrome/browser/ui/ime/ime_warning_bubble.h (right): https://codereview.chromium.org/1724733002/diff/200001/chrome/browser/ui/ime/... chrome/browser/ui/ime/ime_warning_bubble.h:20: // The ImeWarningBubble is self-owned, it deletes itself the widget is closed On 2016/03/15 17:22:18, sky wrote: > the->'when the' Done. https://codereview.chromium.org/1724733002/diff/200001/chrome/browser/ui/ime/... chrome/browser/ui/ime/ime_warning_bubble.h:21: // or fails to create the widget. On 2016/03/15 17:22:18, sky wrote: > AFAICT this never fails. Am I missing something? This fails in ImeWarningBubbleView::Show() when hitting the conditions 'animation_wait_retries_ == kWaitRetries' in Patch Set 11. If the warning bubble has not been shown in a certain time, ImeWarningBubbleView will delete itself. For Patch Set 12, this logic was removed. But I don't know where to handle the situation that the bubble was not successfully shown. Please see the TODO in ImeWarningBubbleView::ShowBubble(). Is there any idea for this problem? https://codereview.chromium.org/1724733002/diff/200001/chrome/browser/ui/ime/... chrome/browser/ui/ime/ime_warning_bubble.h:27: PERMISSION_GRANTED, On 2016/03/15 17:22:18, sky wrote: > PermissionStatus::PERMISSION_GRANTED seems a bit redundant. Did you consider > nuking PERMISSION_? So that here you would have PermissionStatus::GRANTED. Done. https://codereview.chromium.org/1724733002/diff/200001/chrome/browser/ui/ime/... chrome/browser/ui/ime/ime_warning_bubble.h:55: virtual void PressButtonForTesting(bool ok_button_pressed, On 2016/03/15 17:22:18, sky wrote: > PressButtonForTesting is a rather confusing name for this function. I think it's > more of CompleteForTesting or CloseForTesting Done. https://codereview.chromium.org/1724733002/diff/200001/chrome/browser/ui/ime/... chrome/browser/ui/ime/ime_warning_bubble.h:58: virtual ~ImeWarningBubble() {} On 2016/03/15 17:22:18, sky wrote: > destructor before other functions. Done. https://codereview.chromium.org/1724733002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/ime_warning_bubble_browsertest.cc (right): https://codereview.chromium.org/1724733002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_browsertest.cc:63: if (status == ImeWarningBubble::PermissionStatus::PERMISSION_GRANTED || On 2016/03/15 17:22:18, sky wrote: > nit: {} Done. https://codereview.chromium.org/1724733002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/ime_warning_bubble_view.cc (right): https://codereview.chromium.org/1724733002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:36: // The coloumn width of the warning bubble. On 2016/03/15 17:22:18, sky wrote: > column Done. https://codereview.chromium.org/1724733002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:54: checkbox->set_listener(listener); On 2016/03/15 17:22:18, sky wrote: > AFAICT you don't actually need the listener. Done. https://codereview.chromium.org/1724733002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:55: checkbox->SetHorizontalAlignment(gfx::ALIGN_LEFT); On 2016/03/15 17:22:18, sky wrote: > ALIGN_LEFT is the default. I think you can nuke this function and create the > CheckBox directly. Done. Thanks. https://codereview.chromium.org/1724733002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:174: base::ThreadTaskRunnerHandle::Get()->PostDelayedTask( On 2016/03/15 17:22:18, sky wrote: > Delaying until you hope things are better inevitably causes problems. For > example, because you're adding a delay it's entirely possible browser_ may be > destroyed by the time your callback is run. You need a way to directly observe > when the toolbar is done animating, and you need to deal with the browser_ being > deleted before too. I added the function in OnToolbarActionsBarAnimationEnded() in class ToolbarActionsBarObserver. Thus, the bubble could be shown rely on observing the ToolbarActionsBar. But what if the OnToolbarActionsBarAnimationEnded() has never been triggered? The ImeWarningBubbleView will not be deleted since it could not be closed. Is this a problem? As for browser_ being deleted, I added the check before run response_callback_, is this right? https://codereview.chromium.org/1724733002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:191: if (never_show_checkbox_->checked()) On 2016/03/15 17:22:18, sky wrote: > nit: {} in here. Done.
https://codereview.chromium.org/1724733002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/ime_warning_bubble_view.cc (right): https://codereview.chromium.org/1724733002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:174: base::ThreadTaskRunnerHandle::Get()->PostDelayedTask( On 2016/03/16 08:05:48, Azure Wei wrote: > On 2016/03/15 17:22:18, sky wrote: > > Delaying until you hope things are better inevitably causes problems. For > > example, because you're adding a delay it's entirely possible browser_ may be > > destroyed by the time your callback is run. You need a way to directly observe > > when the toolbar is done animating, and you need to deal with the browser_ > being > > deleted before too. > > I added the function in OnToolbarActionsBarAnimationEnded() in class > ToolbarActionsBarObserver. Thus, the bubble could be shown rely on observing the > ToolbarActionsBar. But what if the OnToolbarActionsBarAnimationEnded() has never > been triggered? The ImeWarningBubbleView will not be deleted since it could not > be closed. Is this a problem? > As for browser_ being deleted, I added the check before run response_callback_, > is this right? I see two problems with what you have: . What's to stop multiple ImeWarningBubbleViews from being created and all shown once the animation ends? . null checking the browser does you no good as you never null it out. What you need to do is observe browser_ being destroyed (BrowserListObserver) and then delete this. You only need do this if you're waiting on animation as otherwise the bubble is parented to the browser window.
https://codereview.chromium.org/1724733002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/ime_warning_bubble_view.cc (right): https://codereview.chromium.org/1724733002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:174: base::ThreadTaskRunnerHandle::Get()->PostDelayedTask( On 2016/03/16 15:53:19, sky wrote: > On 2016/03/16 08:05:48, Azure Wei wrote: > > On 2016/03/15 17:22:18, sky wrote: > > > Delaying until you hope things are better inevitably causes problems. For > > > example, because you're adding a delay it's entirely possible browser_ may > be > > > destroyed by the time your callback is run. You need a way to directly > observe > > > when the toolbar is done animating, and you need to deal with the browser_ > > being > > > deleted before too. > > > > I added the function in OnToolbarActionsBarAnimationEnded() in class > > ToolbarActionsBarObserver. Thus, the bubble could be shown rely on observing > the > > ToolbarActionsBar. But what if the OnToolbarActionsBarAnimationEnded() has > never > > been triggered? The ImeWarningBubbleView will not be deleted since it could > not > > be closed. Is this a problem? > > As for browser_ being deleted, I added the check before run > response_callback_, > > is this right? > > I see two problems with what you have: > > . What's to stop multiple ImeWarningBubbleViews from being created and all shown > once the animation ends? > . null checking the browser does you no good as you never null it out. What you > need to do is observe browser_ being destroyed (BrowserListObserver) and then > delete this. You only need do this if you're waiting on animation as otherwise > the bubble is parented to the browser window. Thanks for you replay. The ImeWarningBubbleView was updated to observer BrowserList::OnBrowserRemoved() to delete itself. GetWidget() was used to see whether the bubble was shown or not. And in OnToolbarActionsBarAnimationEnded(), also used GetWidget() to avoid showing multiple bubbles. Could you take another look? Thanks.
https://codereview.chromium.org/1724733002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/ime_warning_bubble_view.cc (right): https://codereview.chromium.org/1724733002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:61: ->GetToolbarView() Do you need to null check GEtToolbarView here like you do on 72? If not, why? https://codereview.chromium.org/1724733002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:64: if (toolbar_cations_bar_) Don't you need to wait until Show() is called before adding observers? https://codereview.chromium.org/1724733002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:72: if (toolbar_view) { Why do you need to do all this when you hvae toolbar_captions_bar_ as a member? https://codereview.chromium.org/1724733002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:175: if (!IsToolbarAnimating()) I would expect an early return if IsToolbarAnimating. https://codereview.chromium.org/1724733002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:199: if (GetWidget()) nit: return GetWidget() && GetWidget()->IsVisible() https://codereview.chromium.org/1724733002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:205: never_show_checkbox_->SetChecked(checked); Who deletes this in the testing code path? https://codereview.chromium.org/1724733002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:218: if ((browser_ == browser) && !GetWidget()) { The !GetWidget() is subtle and worth a comment. I think it may also be racy. I think it better to have a member as to whether you've created the widget. https://codereview.chromium.org/1724733002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:220: response_callback_.Run(ImeWarningBubble::PermissionStatus::ABORTED); Can the response callback be run from the destructor?
https://codereview.chromium.org/1724733002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/ime_warning_bubble_view.cc (right): https://codereview.chromium.org/1724733002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:170: set_parent_window(browser_widget->GetNativeView()); set_parent_window(browser_->window()->GetNativeWindow());?
Also, I don't like all the lookups of BrowserView. If you need the BrowserWindow implementation than the call should be routed to BrowserWindow so you know the BrowserView and can pass it around directly.
https://codereview.chromium.org/1724733002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/ime_warning_bubble_view.cc (right): https://codereview.chromium.org/1724733002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:61: ->GetToolbarView() On 2016/03/17 13:31:45, sky wrote: > Do you need to null check GEtToolbarView here like you do on 72? If not, why? Retested this and it seemed safe to remove the check in the destructor. https://codereview.chromium.org/1724733002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:64: if (toolbar_cations_bar_) On 2016/03/17 13:31:45, sky wrote: > Don't you need to wait until Show() is called before adding observers? Done. https://codereview.chromium.org/1724733002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:72: if (toolbar_view) { On 2016/03/17 13:31:46, sky wrote: > Why do you need to do all this when you hvae toolbar_captions_bar_ as a member? Done. Added toolbar_view_ and toolbar_actions_bar as members. https://codereview.chromium.org/1724733002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:170: set_parent_window(browser_widget->GetNativeView()); On 2016/03/17 13:35:18, sky wrote: > set_parent_window(browser_->window()->GetNativeWindow());? Done. https://codereview.chromium.org/1724733002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:175: if (!IsToolbarAnimating()) On 2016/03/17 13:31:46, sky wrote: > I would expect an early return if IsToolbarAnimating. Done. https://codereview.chromium.org/1724733002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:199: if (GetWidget()) On 2016/03/17 13:31:46, sky wrote: > nit: return GetWidget() && GetWidget()->IsVisible() Done. https://codereview.chromium.org/1724733002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:205: never_show_checkbox_->SetChecked(checked); On 2016/03/17 13:31:46, sky wrote: > Who deletes this in the testing code path? Sorry, do you mean I need to uncheck the check box in the test or delete the check box in the tests? https://codereview.chromium.org/1724733002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:218: if ((browser_ == browser) && !GetWidget()) { On 2016/03/17 13:31:45, sky wrote: > The !GetWidget() is subtle and worth a comment. I think it may also be racy. I > think it better to have a member as to whether you've created the widget. Done. Used the new added data member bubble_has_shown_ to do the judge. https://codereview.chromium.org/1724733002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:220: response_callback_.Run(ImeWarningBubble::PermissionStatus::ABORTED); On 2016/03/17 13:31:45, sky wrote: > Can the response callback be run from the destructor? Done.
On 2016/03/17 13:35:54, sky wrote: > Also, I don't like all the lookups of BrowserView. If you need the BrowserWindow > implementation than the call should be routed to BrowserWindow so you know the > BrowserView and can pass it around directly. I didn't use the methods in BrowserWindow. Instead, ToolbarView, BrowserActionsContainer, ToolbarActionsBar, which were got from browser_, had been used many times. To avoid the duplicated lookup of BrowserView, I added ToolbarView, BrowserActionsContainer and ToolbarActionsBar all as data members in ImeWarningBubbleView. Will this be better?
https://codereview.chromium.org/1724733002/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/ime_warning_bubble_view.cc (right): https://codereview.chromium.org/1724733002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:61: BrowserView::GetBrowserViewForBrowser(browser_)->GetToolbarView(); This is the call that I'm saying I don't like. If you route showing the bubble through BrowserWindow than it will be implemented by BrowserView and you won't need this lookup.
https://codereview.chromium.org/1724733002/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/ime_warning_bubble_view.cc (right): https://codereview.chromium.org/1724733002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:61: BrowserView::GetBrowserViewForBrowser(browser_)->GetToolbarView(); On 2016/03/22 16:22:19, sky wrote: > This is the call that I'm saying I don't like. If you route showing the bubble > through BrowserWindow than it will be implemented by BrowserView and you won't > need this lookup. By routing to BrowserWindow, do you mean add the interface GetToolbarView() in BrowserWindow (see patch set 15)? But I still need to pass browser_ rather than pass BrowserWindow directly, as I need to check the browser_ in OnBrowserRemoved().
On 2016/03/23 22:58:15, Azure Wei wrote: > https://codereview.chromium.org/1724733002/diff/260001/chrome/browser/ui/view... > File chrome/browser/ui/views/ime/ime_warning_bubble_view.cc (right): > > https://codereview.chromium.org/1724733002/diff/260001/chrome/browser/ui/view... > chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:61: > BrowserView::GetBrowserViewForBrowser(browser_)->GetToolbarView(); > On 2016/03/22 16:22:19, sky wrote: > > This is the call that I'm saying I don't like. If you route showing the bubble > > through BrowserWindow than it will be implemented by BrowserView and you won't > > need this lookup. > > By routing to BrowserWindow, do you mean add the interface GetToolbarView() in > BrowserWindow (see patch set 15)? But I still need to pass browser_ rather than > pass BrowserWindow directly, as I need to check the browser_ in > OnBrowserRemoved(). Sorry for not being clear. I mean a function like ShowImeWarningBubble() on BrowserWindow.
On 2016/03/23 23:43:45, sky wrote: > On 2016/03/23 22:58:15, Azure Wei wrote: > > > https://codereview.chromium.org/1724733002/diff/260001/chrome/browser/ui/view... > > File chrome/browser/ui/views/ime/ime_warning_bubble_view.cc (right): > > > > > https://codereview.chromium.org/1724733002/diff/260001/chrome/browser/ui/view... > > chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:61: > > BrowserView::GetBrowserViewForBrowser(browser_)->GetToolbarView(); > > On 2016/03/22 16:22:19, sky wrote: > > > This is the call that I'm saying I don't like. If you route showing the > bubble > > > through BrowserWindow than it will be implemented by BrowserView and you > won't > > > need this lookup. > > > > By routing to BrowserWindow, do you mean add the interface GetToolbarView() in > > BrowserWindow (see patch set 15)? But I still need to pass browser_ rather > than > > pass BrowserWindow directly, as I need to check the browser_ in > > OnBrowserRemoved(). > > Sorry for not being clear. I mean a function like ShowImeWarningBubble() on > BrowserWindow. It's a little complex to add ShowImeWarningBubble() in BrowserWindow and implement it in BrowserView, as the BrowserView is needed in many places, like InitAnchorView(). If implementing ShowBubble() in BrowserView, should I put ImeWarningBubble::InitAnchorView(), ImeWarningBubble::OnToolbarActionsBarAnimationEnded() in BrowserView? It seems strange to put so many bubble related codes in BrowserView. Besides, other bubbles like GlobalErrorBubbleView, ExtensionInstalledBubbleView, all use GetBrowserViewForBrowser(browser_) to get the BrowserView, rather than coding ShowBubble() in BrowserView. Can I follow this idea?
I'm suggesting the following: BrowserWindow::ShowImeWarningBubble, which is implemented by BrowserView::ShowImeWarningBubble. The implementation of BrowserView::ShowImeWarningBubble creates ImeWarningBubbleView and passes 'this' to ImeWarningBubbleView so that ImeWarningBubbleView has a handle to BrowserView and doesn't need to casts. Why? The code clearly needs BrowserView, as such BrowserView should be involved. This way if we had another implementation of BrowserWindow that is not a BrowserView we wouldn't crash. -Scott On Wed, Mar 23, 2016 at 6:09 PM, <azurewei@chromium.org> wrote: > On 2016/03/23 23:43:45, sky wrote: >> On 2016/03/23 22:58:15, Azure Wei wrote: >> > >> > https://codereview.chromium.org/1724733002/diff/260001/chrome/browser/ui/view... >> > File chrome/browser/ui/views/ime/ime_warning_bubble_view.cc (right): >> > >> > >> > https://codereview.chromium.org/1724733002/diff/260001/chrome/browser/ui/view... >> > chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:61: >> > BrowserView::GetBrowserViewForBrowser(browser_)->GetToolbarView(); >> > On 2016/03/22 16:22:19, sky wrote: >> > > This is the call that I'm saying I don't like. If you route showing >> > > the >> bubble >> > > through BrowserWindow than it will be implemented by BrowserView and >> > > you >> won't >> > > need this lookup. >> > >> > By routing to BrowserWindow, do you mean add the interface >> > GetToolbarView() > in >> > BrowserWindow (see patch set 15)? But I still need to pass browser_ >> > rather >> than >> > pass BrowserWindow directly, as I need to check the browser_ in >> > OnBrowserRemoved(). >> >> Sorry for not being clear. I mean a function like ShowImeWarningBubble() >> on >> BrowserWindow. > > It's a little complex to add ShowImeWarningBubble() in BrowserWindow and > implement it in BrowserView, as the BrowserView is needed in many places, > like > InitAnchorView(). > If implementing ShowBubble() in BrowserView, should I put > ImeWarningBubble::InitAnchorView(), > ImeWarningBubble::OnToolbarActionsBarAnimationEnded() in BrowserView? It > seems > strange to put so many bubble related codes in BrowserView. > Besides, other bubbles like GlobalErrorBubbleView, > ExtensionInstalledBubbleView, > all use GetBrowserViewForBrowser(browser_) to get the BrowserView, rather > than > coding ShowBubble() in BrowserView. Can I follow this idea? > > https://codereview.chromium.org/1724733002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/03/24 02:56:39, sky wrote: > I'm suggesting the following: > BrowserWindow::ShowImeWarningBubble, which is implemented by > BrowserView::ShowImeWarningBubble. The implementation of > BrowserView::ShowImeWarningBubble creates ImeWarningBubbleView and > passes 'this' to ImeWarningBubbleView so that ImeWarningBubbleView has > a handle to BrowserView and doesn't need to casts. Why? The code > clearly needs BrowserView, as such BrowserView should be involved. > This way if we had another implementation of BrowserWindow that is not > a BrowserView we wouldn't crash. > > -Scott > > On Wed, Mar 23, 2016 at 6:09 PM, <mailto:azurewei@chromium.org> wrote: > > On 2016/03/23 23:43:45, sky wrote: > >> On 2016/03/23 22:58:15, Azure Wei wrote: > >> > > >> > > > https://codereview.chromium.org/1724733002/diff/260001/chrome/browser/ui/view... > >> > File chrome/browser/ui/views/ime/ime_warning_bubble_view.cc (right): > >> > > >> > > >> > > > https://codereview.chromium.org/1724733002/diff/260001/chrome/browser/ui/view... > >> > chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:61: > >> > BrowserView::GetBrowserViewForBrowser(browser_)->GetToolbarView(); > >> > On 2016/03/22 16:22:19, sky wrote: > >> > > This is the call that I'm saying I don't like. If you route showing > >> > > the > >> bubble > >> > > through BrowserWindow than it will be implemented by BrowserView and > >> > > you > >> won't > >> > > need this lookup. > >> > > >> > By routing to BrowserWindow, do you mean add the interface > >> > GetToolbarView() > > in > >> > BrowserWindow (see patch set 15)? But I still need to pass browser_ > >> > rather > >> than > >> > pass BrowserWindow directly, as I need to check the browser_ in > >> > OnBrowserRemoved(). > >> > >> Sorry for not being clear. I mean a function like ShowImeWarningBubble() > >> on > >> BrowserWindow. > > > > It's a little complex to add ShowImeWarningBubble() in BrowserWindow and > > implement it in BrowserView, as the BrowserView is needed in many places, > > like > > InitAnchorView(). > > If implementing ShowBubble() in BrowserView, should I put > > ImeWarningBubble::InitAnchorView(), > > ImeWarningBubble::OnToolbarActionsBarAnimationEnded() in BrowserView? It > > seems > > strange to put so many bubble related codes in BrowserView. > > Besides, other bubbles like GlobalErrorBubbleView, > > ExtensionInstalledBubbleView, > > all use GetBrowserViewForBrowser(browser_) to get the BrowserView, rather > > than > > coding ShowBubble() in BrowserView. Can I follow this idea? > > > > https://codereview.chromium.org/1724733002/ > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. > Many thanks for you detailed explanation. Please review the latest patch set. Thank you!
https://codereview.chromium.org/1724733002/diff/320001/chrome/browser/ui/brow... File chrome/browser/ui/browser_window.h (right): https://codereview.chromium.org/1724733002/diff/320001/chrome/browser/ui/brow... chrome/browser/ui/browser_window.h:71: enum class ImeWarningBubblePermissionStatus { Move the enum to the ime code and forward declare the enum here. https://codereview.chromium.org/1724733002/diff/320001/chrome/browser/ui/brow... chrome/browser/ui/browser_window.h:398: ImeWarningBubbleResponseCallback& callback) {} I would expect this to be pure virtual. https://codereview.chromium.org/1724733002/diff/320001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/1724733002/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view.cc:2655: void BrowserView::ShowImeWarningBubble( Style guide says declaration and definition order should match. https://codereview.chromium.org/1724733002/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view.cc:2661: new ImeWarningBubbleView(extension, this, callback); As the caller doesn't own the ImeWarningBubbleView, and there isn't really anything interesting that can be done with ImeWarningBubbleView here, how about a static function: ImeWarningBubbleView;:Show(extension, this, callback); ? That means you can get rid of ime_warning_bubble_for_test_. If you need the pointer for testing make ImeWarningBubbleView hold the current instance in a variable and add a private getter your test can use to get it. https://codereview.chromium.org/1724733002/diff/320001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/ime_warning_bubble_view.cc (right): https://codereview.chromium.org/1724733002/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:197: if ((browser_view_->browser() == browser) && !bubble_has_shown_) I'm a bit worried about whether browser_view_ is valid at the time OnBrowserRemoved is called. Did you verify it is? A more sane approach is to have BrowserView own ime_warning_bubble_view, but that's a bit more complicated given you want it to destroy on hide.
https://codereview.chromium.org/1724733002/diff/320001/chrome/browser/ui/brow... File chrome/browser/ui/browser_window.h (right): https://codereview.chromium.org/1724733002/diff/320001/chrome/browser/ui/brow... chrome/browser/ui/browser_window.h:71: enum class ImeWarningBubblePermissionStatus { On 2016/03/24 18:56:23, sky wrote: > Move the enum to the ime code and forward declare the enum here. Done. https://codereview.chromium.org/1724733002/diff/320001/chrome/browser/ui/brow... chrome/browser/ui/browser_window.h:398: ImeWarningBubbleResponseCallback& callback) {} On 2016/03/24 18:56:23, sky wrote: > I would expect this to be pure virtual. Done. https://codereview.chromium.org/1724733002/diff/320001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/1724733002/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view.cc:2655: void BrowserView::ShowImeWarningBubble( On 2016/03/24 18:56:23, sky wrote: > Style guide says declaration and definition order should match. Done. https://codereview.chromium.org/1724733002/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view.cc:2661: new ImeWarningBubbleView(extension, this, callback); On 2016/03/24 18:56:23, sky wrote: > As the caller doesn't own the ImeWarningBubbleView, and there isn't really > anything interesting that can be done with ImeWarningBubbleView here, how about > a static function: > > ImeWarningBubbleView;:Show(extension, this, callback); ? > > That means you can get rid of ime_warning_bubble_for_test_. If you need the > pointer for testing make ImeWarningBubbleView hold the current instance in a > variable and add a private getter your test can use to get it. Done. Thanks. https://codereview.chromium.org/1724733002/diff/320001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/ime_warning_bubble_view.cc (right): https://codereview.chromium.org/1724733002/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:197: if ((browser_view_->browser() == browser) && !bubble_has_shown_) On 2016/03/24 18:56:23, sky wrote: > I'm a bit worried about whether browser_view_ is valid at the time > OnBrowserRemoved is called. Did you verify it is? > A more sane approach is to have BrowserView own ime_warning_bubble_view, but > that's a bit more complicated given you want it to destroy on hide. Oh, tests show that browser_view_->browser() is nullptr when OnBrowserRemoved() called. I added a new private variable browser_ to save the pointer at the constructor, is this solution OK?
You'll need to update the mac side, but otherwise this LGTM https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/ime_warning_bubble_view.h (right): https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.h:37: ImeWarningBubbleView(const extensions::Extension* extension, Make this private if you can. https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.h:87: // OnBrowserRemoved(); This comment isn't particularly helpful as you don't say why this doesn't use browser_view_->browser(). That browser_view_->browser() may be null is why it needs to be cached and should be documented.
https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/extensi... File chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc (right): https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:40: const char kErrorNotGetUserPermission[] = kErrorPermissionDenied https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:41: "User permission required but not gotten for activating the extension."; Let's rephrase this as "User denied permission". https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:48: const char kPrefWarningBubbleNeverShow[] = "warning_bubble_never_show"; let's add "ime" somewhere in here - "skip_ime_warning_bubble"? https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:205: if (!user_gesture()) nit: put this above disable_bubble_for_testing_ check https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:241: let's add a DCHECK(status == GRANTED || status == GRANTED_AND_NEVER_SHOW) so that if we ever add a new enum we remember to update this. https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/extensi... File chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h (right): https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h:102: // Called when user finishes interacting with the warning bubble. nit: Called when *the* user... https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h:103: // |ok_button_pressed| indicates whether the 'OK' button is pressed by the This comment needs updating. https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h:106: void OnGotUserResponse(const ImeWarningBubblePermissionStatus& status); OnGotUserResponse sounds a little funny - maybe just "OnPermissionBubbleFinished"? https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/ui/brow... File chrome/browser/ui/browser_window.h (right): https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/ui/brow... chrome/browser/ui/browser_window.h:73: base::Callback<void(const ImeWarningBubblePermissionStatus& status)>; Why does the enum need to be const&? It's not any cheaper. Is it necessary for the forward declaration? https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/ui/brow... chrome/browser/ui/browser_window.h:391: ImeWarningBubbleResponseCallback& callback) = 0; const & callback https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view.cc:2542: // The ImeWarningBubbleView is self-owned, it deletes itself when the widget This comment seems a little out of place, since there's not really any memory to manage here. I think this might be better in the ShowBubble() method. https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/ime_warning_bubble_browsertest.cc (right): https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_browsertest.cc:62: EXPECT_TRUE(!!ime_warning_bubble_); you don't need the !! here (or below). Also, since you dereference |ime_warning_bubble_| right beneath these, these should be ASSERTs. https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/ime_warning_bubble_view.cc (right): https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:126: response_callback_.Run(ImeWarningBubblePermissionStatus::ABORTED); Hmm... Doesn't this mean the response_callback_ can run multiple times? You never reset it in Accept() or Cancel(), and then following that the bubble is destroyed. That can lead to leaks and quite a bit of weirdness in classes that only expect the callback to be fired once. https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:127: toolbar_actions_bar_->RemoveObserver(this); Prefer scoped observers here. https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:127: toolbar_actions_bar_->RemoveObserver(this); Are we quite certain that this object won't outlive the ToolbarActionsBar, even in the case of the browser being removed? If not, it'll be a uaf here. https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:198: ToolbarActionsBar::disable_animations_for_testing_) Hmm... why do we need this check? If ToolbarActionsBar::disable_animations_for_testing_ is true, then container_->animating() should always return false. So this whole function should just be: return anchor_to_browser_action && container_->animating(); https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:204: return (GetWidget() && GetWidget()->IsVisible()); I think we should just move this to the test - GetWidget() and IsVisible() are already public. https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:208: never_show_checkbox_->SetChecked(checked); I'd probably also say these should just be in the test, too - the test can set the checkbox as checked and then call Accept() or Cancel().
https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/ime_warning_bubble_view.cc (right): https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:19: #include "chrome/common/extensions/api/omnibox/omnibox_handler.h" Also, please trim your includes here.
The CQ bit was checked by azurewei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1724733002/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1724733002/380001
The CQ bit was unchecked by azurewei@chromium.org
The CQ bit was checked by azurewei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1724733002/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1724733002/400001
The CQ bit was unchecked by azurewei@chromium.org
https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/extensi... File chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc (right): https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:40: const char kErrorNotGetUserPermission[] = On 2016/03/25 16:35:10, Devlin wrote: > kErrorPermissionDenied Done. https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:41: "User permission required but not gotten for activating the extension."; On 2016/03/25 16:35:10, Devlin wrote: > Let's rephrase this as "User denied permission". Done. https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:48: const char kPrefWarningBubbleNeverShow[] = "warning_bubble_never_show"; On 2016/03/25 16:35:10, Devlin wrote: > let's add "ime" somewhere in here - "skip_ime_warning_bubble"? Done. https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:205: if (!user_gesture()) On 2016/03/25 16:35:10, Devlin wrote: > nit: put this above disable_bubble_for_testing_ check Done. https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:241: On 2016/03/25 16:35:10, Devlin wrote: > let's add a DCHECK(status == GRANTED || status == GRANTED_AND_NEVER_SHOW) so > that if we ever add a new enum we remember to update this. Done. https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/extensi... File chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h (right): https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h:102: // Called when user finishes interacting with the warning bubble. On 2016/03/25 16:35:10, Devlin wrote: > nit: Called when *the* user... Done. https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h:103: // |ok_button_pressed| indicates whether the 'OK' button is pressed by the On 2016/03/25 16:35:10, Devlin wrote: > This comment needs updating. Updated. https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h:106: void OnGotUserResponse(const ImeWarningBubblePermissionStatus& status); On 2016/03/25 16:35:10, Devlin wrote: > OnGotUserResponse sounds a little funny - maybe just > "OnPermissionBubbleFinished"? Done. https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/ui/brow... File chrome/browser/ui/browser_window.h (right): https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/ui/brow... chrome/browser/ui/browser_window.h:73: base::Callback<void(const ImeWarningBubblePermissionStatus& status)>; On 2016/03/25 16:35:10, Devlin wrote: > Why does the enum need to be const&? It's not any cheaper. Is it necessary for > the forward declaration? Changed 'const ImeWarningBubblePermissionStatus&' with ImeWarningBubblePermissionStatus and put the 'using ImeWarningBubbleResponseCallback=...' in ime_warning_bubble_view.h. https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/ui/brow... chrome/browser/ui/browser_window.h:391: ImeWarningBubbleResponseCallback& callback) = 0; On 2016/03/25 16:35:10, Devlin wrote: > const & callback Done. https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view.cc:2542: // The ImeWarningBubbleView is self-owned, it deletes itself when the widget On 2016/03/25 16:35:10, Devlin wrote: > This comment seems a little out of place, since there's not really any memory to > manage here. I think this might be better in the ShowBubble() method. Done. https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/ime_warning_bubble_browsertest.cc (right): https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_browsertest.cc:62: EXPECT_TRUE(!!ime_warning_bubble_); On 2016/03/25 16:35:10, Devlin wrote: > you don't need the !! here (or below). Also, since you dereference > |ime_warning_bubble_| right beneath these, these should be ASSERTs. Done. https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/ime_warning_bubble_view.cc (right): https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:19: #include "chrome/common/extensions/api/omnibox/omnibox_handler.h" On 2016/03/25 16:40:20, Devlin wrote: > Also, please trim your includes here. Done. https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:126: response_callback_.Run(ImeWarningBubblePermissionStatus::ABORTED); On 2016/03/25 16:35:11, Devlin wrote: > Hmm... Doesn't this mean the response_callback_ can run multiple times? You > never reset it in Accept() or Cancel(), and then following that the bubble is > destroyed. That can lead to leaks and quite a bit of weirdness in classes that > only expect the callback to be fired once. Added a variable |response_callback_run_| to make sure |reponse_callback_| won't run more than one time. Hope that looks better. https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:127: toolbar_actions_bar_->RemoveObserver(this); On 2016/03/25 16:35:10, Devlin wrote: > Prefer scoped observers here. Done. https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:127: toolbar_actions_bar_->RemoveObserver(this); On 2016/03/25 16:35:10, Devlin wrote: > Are we quite certain that this object won't outlive the ToolbarActionsBar, even > in the case of the browser being removed? If not, it'll be a uaf here. Yeah, tested that toolbar_actions_bar_ won't outlive in here. As the bubble could be deleted in more than one places, like OnWidgetActivationChanged(), OnWidgetClosed(), OnBrowserRemoved(), it's more convenient to remove the observer all in the destructor. https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:198: ToolbarActionsBar::disable_animations_for_testing_) On 2016/03/25 16:35:10, Devlin wrote: > Hmm... why do we need this check? If > ToolbarActionsBar::disable_animations_for_testing_ is true, then > container_->animating() should always return false. > > So this whole function should just be: > return anchor_to_browser_action && container_->animating(); container_->animating() didn't always return true with setting |ToolbarActionsBar::disable_animations_for_testing_|=true. Changed this as 'anchor_to_browser_action_ && !ToolbarActionsBar::disable_animations_for_testing_ && container_->animating()', is this OK? https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:204: return (GetWidget() && GetWidget()->IsVisible()); On 2016/03/25 16:35:10, Devlin wrote: > I think we should just move this to the test - GetWidget() and IsVisible() are > already public. Done. https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:208: never_show_checkbox_->SetChecked(checked); On 2016/03/25 16:35:10, Devlin wrote: > I'd probably also say these should just be in the test, too - the test can set > the checkbox as checked and then call Accept() or Cancel(). Done. https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/ime_warning_bubble_view.h (right): https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.h:37: ImeWarningBubbleView(const extensions::Extension* extension, On 2016/03/25 15:41:45, sky wrote: > Make this private if you can. Done. https://codereview.chromium.org/1724733002/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.h:87: // OnBrowserRemoved(); On 2016/03/25 15:41:46, sky wrote: > This comment isn't particularly helpful as you don't say why this doesn't use > browser_view_->browser(). That browser_view_->browser() may be null is why it > needs to be cached and should be documented. Updated.
https://codereview.chromium.org/1724733002/diff/400001/chrome/browser/extensi... File chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc (right): https://codereview.chromium.org/1724733002/diff/400001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:228: browser->window()->ShowImeWarningBubble(extension(), callback); nit: I think that inlining callback here would make this more readable. https://codereview.chromium.org/1724733002/diff/400001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right): https://codereview.chromium.org/1724733002/diff/400001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_cocoa.mm:858: // The IME warning bubble will only be shown on Linux & Windows. This should be a NOTREACHED() https://codereview.chromium.org/1724733002/diff/400001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/ime_warning_bubble_browsertest.cc (right): https://codereview.chromium.org/1724733002/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_browsertest.cc:28: base::Callback<void(ImeWarningBubblePermissionStatus status)> callback; this should be callback_ https://codereview.chromium.org/1724733002/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_browsertest.cc:42: extension_ = ExtensionBrowserTest::LoadExtension( note that there's no reason for ExtensionBrowserTest:: prefix here. https://codereview.chromium.org/1724733002/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_browsertest.cc:46: ToolbarActionsBar::disable_animations_for_testing_ = true; It's possible that the reason container is returning true for animating in ImeWarningBubble is because adding an extension starts an animation, and we do that before we disable animations. Try putting this as the first line in this method. Also, you need to call the super SetUpOnMainThread() here. So this function should be something like: void SetUpOnMainThread() { ToolbarActionsBar::disable_animations_for_testing_ = true; ExtensionBrowserTest::SetUpOnMainThread(); extension_ = LoadExtension(...); callback_ = base::Bind(...); } https://codereview.chromium.org/1724733002/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_browsertest.cc:77: IN_PROC_BROWSER_TEST_F(ImeWarningBubbleBrowserTest, ShowBubble) { With the other two tests, we don't need this one, right? (There's no way the other two could succeed and this one fail) https://codereview.chromium.org/1724733002/diff/400001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/ime_warning_bubble_view.cc (right): https://codereview.chromium.org/1724733002/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:10: #include "chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h" It looks like there's still a few includes here you don't need. Please trim the whole list down to what you use. https://codereview.chromium.org/1724733002/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:67: response_callback_.Run(ImeWarningBubblePermissionStatus::DENIED); Don't we need to indicate response_callback_run_ here, too? An easier way might be to use base::ResetAndReturn(&callback_), and then check for callback_.IsNull() in the dtor. https://codereview.chromium.org/1724733002/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:129: toolbar_actions_bar_observer_.RemoveAll(); This happens by default in the scoped observer dtor. https://codereview.chromium.org/1724733002/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:200: !ToolbarActionsBar::disable_animations_for_testing_ && Hmm... container *shouldn't* ever return true for animating() if animation is disabled. Can you check what I suggested in the test, and if it's still returning true, add a TODO here to investigate? https://codereview.chromium.org/1724733002/diff/400001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/ime_warning_bubble_view.h (right): https://codereview.chromium.org/1724733002/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.h:12: #include "chrome/browser/ui/browser_window.h" Do we need this? https://codereview.chromium.org/1724733002/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.h:15: #include "ui/views/controls/button/button.h" Do we need this? https://codereview.chromium.org/1724733002/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.h:52: // chrome::BrowserListObserver nit: end in a ':' https://codereview.chromium.org/1724733002/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.h:57: FRIEND_TEST_ALL_PREFIXES(ImeWarningBubbleBrowserTest, ShowBubble); Do we need to friend the specific tests? It looks like the bubble manipulation is all done from the root ImeWarningBubbleBrowserTest class. https://codereview.chromium.org/1724733002/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.h:86: // Whether anchors the bubble to the browser action or the app menu. nit: this doesn't read quite right. Maybe "True if bubble anchors to the browser action of the extension."
https://codereview.chromium.org/1724733002/diff/400001/chrome/browser/extensi... File chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc (right): https://codereview.chromium.org/1724733002/diff/400001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:228: browser->window()->ShowImeWarningBubble(extension(), callback); On 2016/03/28 20:30:24, Devlin wrote: > nit: I think that inlining callback here would make this more readable. Done. https://codereview.chromium.org/1724733002/diff/400001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right): https://codereview.chromium.org/1724733002/diff/400001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_cocoa.mm:858: // The IME warning bubble will only be shown on Linux & Windows. On 2016/03/28 20:30:24, Devlin wrote: > This should be a NOTREACHED() Done. https://codereview.chromium.org/1724733002/diff/400001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/ime_warning_bubble_browsertest.cc (right): https://codereview.chromium.org/1724733002/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_browsertest.cc:28: base::Callback<void(ImeWarningBubblePermissionStatus status)> callback; On 2016/03/28 20:30:24, Devlin wrote: > this should be callback_ Done. https://codereview.chromium.org/1724733002/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_browsertest.cc:42: extension_ = ExtensionBrowserTest::LoadExtension( On 2016/03/28 20:30:24, Devlin wrote: > note that there's no reason for ExtensionBrowserTest:: prefix here. Done. https://codereview.chromium.org/1724733002/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_browsertest.cc:46: ToolbarActionsBar::disable_animations_for_testing_ = true; On 2016/03/28 20:30:24, Devlin wrote: > It's possible that the reason container is returning true for animating in > ImeWarningBubble is because adding an extension starts an animation, and we do > that before we disable animations. Try putting this as the first line in this > method. > > Also, you need to call the super SetUpOnMainThread() here. So this function > should be something like: > void SetUpOnMainThread() { > ToolbarActionsBar::disable_animations_for_testing_ = true; > ExtensionBrowserTest::SetUpOnMainThread(); > extension_ = LoadExtension(...); > callback_ = base::Bind(...); > } Oh, the tests passed with this order. Many thanks! https://codereview.chromium.org/1724733002/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_browsertest.cc:77: IN_PROC_BROWSER_TEST_F(ImeWarningBubbleBrowserTest, ShowBubble) { On 2016/03/28 20:30:24, Devlin wrote: > With the other two tests, we don't need this one, right? (There's no way the > other two could succeed and this one fail) Done. https://codereview.chromium.org/1724733002/diff/400001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/ime_warning_bubble_view.cc (right): https://codereview.chromium.org/1724733002/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:10: #include "chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h" On 2016/03/28 20:30:24, Devlin wrote: > It looks like there's still a few includes here you don't need. Please trim the > whole list down to what you use. Done. https://codereview.chromium.org/1724733002/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:67: response_callback_.Run(ImeWarningBubblePermissionStatus::DENIED); On 2016/03/28 20:30:24, Devlin wrote: > Don't we need to indicate response_callback_run_ here, too? > > An easier way might be to use base::ResetAndReturn(&callback_), and then check > for callback_.IsNull() in the dtor. It's really easier to use base::ResetAndReturn() here, thanks! https://codereview.chromium.org/1724733002/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:129: toolbar_actions_bar_observer_.RemoveAll(); On 2016/03/28 20:30:24, Devlin wrote: > This happens by default in the scoped observer dtor. Removed. https://codereview.chromium.org/1724733002/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:200: !ToolbarActionsBar::disable_animations_for_testing_ && On 2016/03/28 20:30:24, Devlin wrote: > Hmm... container *shouldn't* ever return true for animating() if animation is > disabled. Can you check what I suggested in the test, and if it's still > returning true, add a TODO here to investigate? Updated the test and removed this condition here. https://codereview.chromium.org/1724733002/diff/400001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/ime_warning_bubble_view.h (right): https://codereview.chromium.org/1724733002/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.h:12: #include "chrome/browser/ui/browser_window.h" On 2016/03/28 20:30:24, Devlin wrote: > Do we need this? Deleted. https://codereview.chromium.org/1724733002/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.h:15: #include "ui/views/controls/button/button.h" On 2016/03/28 20:30:24, Devlin wrote: > Do we need this? Deleted. https://codereview.chromium.org/1724733002/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.h:52: // chrome::BrowserListObserver On 2016/03/28 20:30:24, Devlin wrote: > nit: end in a ':' Done. https://codereview.chromium.org/1724733002/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.h:57: FRIEND_TEST_ALL_PREFIXES(ImeWarningBubbleBrowserTest, ShowBubble); On 2016/03/28 20:30:25, Devlin wrote: > Do we need to friend the specific tests? It looks like the bubble manipulation > is all done from the root ImeWarningBubbleBrowserTest class. Moved 'ime_warning_bubble_ = ImeWarningBubbleView::ime_warning_bubble_for_test_;' to SetUpOnMainThread() and removed these FRIEND_TEST_ALL_PREFIXES here. https://codereview.chromium.org/1724733002/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.h:86: // Whether anchors the bubble to the browser action or the app menu. On 2016/03/28 20:30:24, Devlin wrote: > nit: this doesn't read quite right. Maybe > "True if bubble anchors to the browser action of the extension." Done.
lgtm with a question https://codereview.chromium.org/1724733002/diff/440001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/ime_warning_bubble_view.cc (right): https://codereview.chromium.org/1724733002/diff/440001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:52: if (response_callback_.is_null()) When would this happen?
https://codereview.chromium.org/1724733002/diff/440001/chrome/browser/ui/view... File chrome/browser/ui/views/ime/ime_warning_bubble_view.cc (right): https://codereview.chromium.org/1724733002/diff/440001/chrome/browser/ui/view... chrome/browser/ui/views/ime/ime_warning_bubble_view.cc:52: if (response_callback_.is_null()) On 2016/03/29 22:39:54, Devlin wrote: > When would this happen? Em, I've never met this happen. But the codes run to |response_callback_|.is_null()==true in Cancel() in tests. I added the check here just in case. I would test more and remove this if it never run.
https://codereview.chromium.org/1724733002/diff/440001/chrome/browser/extensi... File chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc (left): https://codereview.chromium.org/1724733002/diff/440001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:41: } // namespace why remove the comment?
https://codereview.chromium.org/1724733002/diff/440001/chrome/browser/extensi... File chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc (left): https://codereview.chromium.org/1724733002/diff/440001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc:41: } // namespace On 2016/03/30 03:34:48, Shu Chen wrote: > why remove the comment? This comment should be in line 107.
lgtm
The CQ bit was checked by azurewei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, shuchen@chromium.org, rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/1724733002/#ps460001 (title: "Fix patch conflict.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1724733002/460001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1724733002/460001
Message was sent while issue was closed.
Description was changed from ========== Show a warning bubble when the IME extension is activated. This cl implements showing a warning bubble when an IME extension is calling chrome.input.ime.activate() API, according to the API proposal: https://goo.gl/IeN7xE. The input.ime.activate() API will not respond util user finishes interacting with the bubble. Only when user presses the 'OK' button, the extension call be successfully activated and get an input.ime.onActivate() event. If user checks the 'Never show this again.' check box when clicking 'OK' button, the extension could be directly activated next time if the API is called from a user action like clicking buttons. This cl adds the following new classes: ImeWarningBubble: The interface or the IME warning bubble. ImeWarningBubbleView: The implementation for the IME warning bubble. Provides warning information to the user upon the activation of an IME extension. BUG=517773 TEST=BrowserTest.ImeActivatedBubbleBrowserTest ========== to ========== Show a warning bubble when the IME extension is activated. This cl implements showing a warning bubble when an IME extension is calling chrome.input.ime.activate() API, according to the API proposal: https://goo.gl/IeN7xE. The input.ime.activate() API will not respond util user finishes interacting with the bubble. Only when user presses the 'OK' button, the extension call be successfully activated and get an input.ime.onActivate() event. If user checks the 'Never show this again.' check box when clicking 'OK' button, the extension could be directly activated next time if the API is called from a user action like clicking buttons. This cl adds the following new classes: ImeWarningBubble: The interface or the IME warning bubble. ImeWarningBubbleView: The implementation for the IME warning bubble. Provides warning information to the user upon the activation of an IME extension. BUG=517773 TEST=BrowserTest.ImeActivatedBubbleBrowserTest ==========
Message was sent while issue was closed.
Committed patchset #24 (id:460001)
Message was sent while issue was closed.
Description was changed from ========== Show a warning bubble when the IME extension is activated. This cl implements showing a warning bubble when an IME extension is calling chrome.input.ime.activate() API, according to the API proposal: https://goo.gl/IeN7xE. The input.ime.activate() API will not respond util user finishes interacting with the bubble. Only when user presses the 'OK' button, the extension call be successfully activated and get an input.ime.onActivate() event. If user checks the 'Never show this again.' check box when clicking 'OK' button, the extension could be directly activated next time if the API is called from a user action like clicking buttons. This cl adds the following new classes: ImeWarningBubble: The interface or the IME warning bubble. ImeWarningBubbleView: The implementation for the IME warning bubble. Provides warning information to the user upon the activation of an IME extension. BUG=517773 TEST=BrowserTest.ImeActivatedBubbleBrowserTest ========== to ========== Show a warning bubble when the IME extension is activated. This cl implements showing a warning bubble when an IME extension is calling chrome.input.ime.activate() API, according to the API proposal: https://goo.gl/IeN7xE. The input.ime.activate() API will not respond util user finishes interacting with the bubble. Only when user presses the 'OK' button, the extension call be successfully activated and get an input.ime.onActivate() event. If user checks the 'Never show this again.' check box when clicking 'OK' button, the extension could be directly activated next time if the API is called from a user action like clicking buttons. This cl adds the following new classes: ImeWarningBubble: The interface or the IME warning bubble. ImeWarningBubbleView: The implementation for the IME warning bubble. Provides warning information to the user upon the activation of an IME extension. BUG=517773 TEST=BrowserTest.ImeActivatedBubbleBrowserTest Committed: https://crrev.com/29221c5b81d089742ff6a4e92a35bbcf8fa98b8d Cr-Commit-Position: refs/heads/master@{#383916} ==========
Message was sent while issue was closed.
Patchset 24 (id:??) landed as https://crrev.com/29221c5b81d089742ff6a4e92a35bbcf8fa98b8d Cr-Commit-Position: refs/heads/master@{#383916} |
