Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1268163002:
Dispatch automation events to the last used profile. (Closed)
|
Created:
5 years, 4 months ago by dmazzoni Modified:
5 years, 3 months ago CC:
chromium-apps-reviews_chromium.org, chromium-reviews, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDispatch automation events to the last used profile.
Specifically, on Chrome OS only, when the same extension
(like ChromeVox) is loaded into more than one profile, send
a flag along with the event and only pass the event to the
JavaScript layer for the active profile. The other inactive
ones will still update their accessibility trees silently in
the background.
BUG=515538
Committed: https://crrev.com/36eee6c86d20119bab09d7cbe3ab42622a794531
Cr-Commit-Position: refs/heads/master@{#348570}
Patch Set 1 #Patch Set 2 : Better fix #Patch Set 3 : Better solution #Patch Set 4 : Fix crash #
Total comments: 13
Patch Set 5 : Address feedback, fix compile error #Patch Set 6 : Rebase #Patch Set 7 : Add to owners file #
Total comments: 2
Patch Set 8 : const auto& #Patch Set 9 : Fix rebase error, add back include of profile_manager.h #Patch Set 10 : Rebase #Patch Set 11 : Rebase #
Total comments: 1
Depends on Patchset: Messages
Total messages: 34 (9 generated)
dmazzoni@chromium.org changed reviewers: + dtseng@chromium.org, plundblad@chromium.org
This was the idea we talked about, but it could cause problems because then the 'other' ChromeVox profile will get out of sync, i.e. its accessibility trees won't be consistent anymore. Perhaps we should just pass a flag, is_active_profile, and then if it's not the active profile the automation API can update its data structures but not fire an event?
dmazzoni@chromium.org writes: > Reviewers: David Tseng, Peter Lundblad, > > Message: > This was the idea we talked about, but it could cause problems because then > the > 'other' ChromeVox profile will get out of sync, i.e. its accessibility trees > won't be consistent anymore. > > Perhaps we should just pass a flag, is_active_profile, and then if it's not > the > active profile the automation API can update its data structures but not > fire an > event? > What if we just resent the entire tree whenever the active profile changes (to all listeners in that profile)? > > Description: > Dispatch automation events to the last used profile. > > BUG=515538 > > Please review this at https://codereview.chromium.org/1268163002/ > > Base URL: https://chromium.googlesource.com/chromium/src.git@master > > Affected files (+46, -6 lines): > M > chrome/browser/extensions/api/automation_internal/automation_event_router.h > M > chrome/browser/extensions/api/automation_internal/automation_event_router.cc > M > chrome/browser/extensions/api/automation_internal/automation_internal_api.cc > M chrome/renderer/extensions/automation_internal_custom_bindings.cc > > > Index: > chrome/browser/extensions/api/automation_internal/automation_event_router.cc > diff --git > a/chrome/browser/extensions/api/automation_internal/automation_event_router.cc > b/chrome/browser/extensions/api/automation_internal/automation_event_router.cc > index > 7be3332d2e80e193420d070684884a7a387a7097..8b1120305fab852144bd3e41f25e2948d08b2b39 > 100644 > --- > a/chrome/browser/extensions/api/automation_internal/automation_event_router.cc > +++ > b/chrome/browser/extensions/api/automation_internal/automation_event_router.cc > @@ -11,6 +11,7 @@ > #include "base/stl_util.h" > #include "base/values.h" > #include "chrome/browser/accessibility/ax_tree_id_registry.h" > +#include "chrome/browser/profiles/profile_manager.h" > #include "chrome/common/extensions/api/automation_internal.h" > #include "chrome/common/extensions/chrome_extension_messages.h" > #include "content/public/browser/notification_service.h" > @@ -40,30 +41,58 @@ AutomationEventRouter::~AutomationEventRouter() { > } > > void AutomationEventRouter::RegisterListenerForOneTree( > + const ExtensionId& extension_id, > int listener_process_id, > int listener_routing_id, > int source_ax_tree_id) { > - Register(listener_process_id, listener_routing_id, source_ax_tree_id, > false); > + Register(extension_id, > + listener_process_id, > + listener_routing_id, > + source_ax_tree_id, > + false); > } > > void AutomationEventRouter::RegisterListenerWithDesktopPermission( > + const ExtensionId& extension_id, > int listener_process_id, > int listener_routing_id) { > - Register(listener_process_id, listener_routing_id, 0, true); > + Register(extension_id, > + listener_process_id, > + listener_routing_id, > + 0, > + true); > } > > void AutomationEventRouter::DispatchAccessibilityEvent( > const ExtensionMsg_AccessibilityEventParams& params) { > + // Build up a map from extension id to the listener where we > + // should send the event. > + std::map<ExtensionId, const AutomationListener*> listeners; > + Profile* last_used_profile = ProfileManager::GetLastUsedProfile(); > for (const auto& listener : listeners_) { > + // Reject listeners that don't want to listen to this tree. > if (!listener.desktop && > listener.tree_ids.find(params.tree_id) == listener.tree_ids.end()) > { > continue; > } > > + // Choose at most one listener per extension ID, > + // favoring the last used profile. > content::RenderProcessHost* rph = > content::RenderProcessHost::FromID(listener.process_id); > - rph->Send(new ExtensionMsg_AccessibilityEvent(listener.routing_id, > - params)); > + bool is_current_profile = (rph->GetBrowserContext() == > last_used_profile); > + auto iter = listeners.find(listener.extension_id); > + if (iter == listeners.end() || is_current_profile) > + listeners[listener.extension_id] = &listener; > + } > + > + // Send the event to the selected listeners. > + for (auto iter : listeners) { > + const AutomationListener* listener = iter.second; > + content::RenderProcessHost* rph = > + content::RenderProcessHost::FromID(listener->process_id); > + rph->Send(new ExtensionMsg_AccessibilityEvent(listener->routing_id, > + params)); > } > } > > @@ -87,6 +116,7 @@ > AutomationEventRouter::AutomationListener::~AutomationListener() { > } > > void AutomationEventRouter::Register( > + const ExtensionId& extension_id, > int listener_process_id, > int listener_routing_id, > int ax_tree_id, > @@ -103,6 +133,7 @@ void AutomationEventRouter::Register( > // Add a new entry if we don't have one with that process and routing id. > if (iter == listeners_.end()) { > AutomationListener listener; > + listener.extension_id = extension_id; > listener.routing_id = listener_routing_id; > listener.process_id = listener_process_id; > listener.desktop = desktop; > Index: > chrome/browser/extensions/api/automation_internal/automation_event_router.h > diff --git > a/chrome/browser/extensions/api/automation_internal/automation_event_router.h > b/chrome/browser/extensions/api/automation_internal/automation_event_router.h > index > 1389b43205944e6b76b9c728074c2a54b3bd1f41..97a1307b5e645b0c2dc72c2eae3bf5895ec83aa2 > 100644 > --- > a/chrome/browser/extensions/api/automation_internal/automation_event_router.h > +++ > b/chrome/browser/extensions/api/automation_internal/automation_event_router.h > @@ -32,14 +32,16 @@ class AutomationEventRouter : public > content::NotificationObserver { > // wants to receive automation events from the accessibility tree > indicated > // by |source_ax_tree_id|. Automation events are forwarded from now on > // until the listener process dies. > - void RegisterListenerForOneTree(int listener_process_id, > + void RegisterListenerForOneTree(const ExtensionId& extension_id, > + int listener_process_id, > int listener_routing_id, > int source_ax_tree_id); > > // Indicates that the listener at |listener_process_id|, | > listener_routing_id| > // wants to receive automation events from all accessibility trees > because > // it has Desktop permission. > - void RegisterListenerWithDesktopPermission(int listener_process_id, > + void RegisterListenerWithDesktopPermission(const ExtensionId& > extension_id, > + int listener_process_id, > int listener_routing_id); > > void DispatchAccessibilityEvent( > @@ -56,6 +58,7 @@ class AutomationEventRouter : public > content::NotificationObserver { > AutomationListener(); > ~AutomationListener(); > > + ExtensionId extension_id; > int routing_id; > int process_id; > bool desktop; > @@ -66,6 +69,7 @@ class AutomationEventRouter : public > content::NotificationObserver { > ~AutomationEventRouter() override; > > void Register( > + const ExtensionId& extension_id, > int listener_process_id, > int listener_routing_id, > int source_ax_tree_id, > Index: > chrome/browser/extensions/api/automation_internal/automation_internal_api.cc > diff --git > a/chrome/browser/extensions/api/automation_internal/automation_internal_api.cc > b/chrome/browser/extensions/api/automation_internal/automation_internal_api.cc > index > 58efba5f7592f7e215f84d71488abb3f54384211..90b26b86ffad32827345852d266a81678efd80c1 > 100644 > --- > a/chrome/browser/extensions/api/automation_internal/automation_internal_api.cc > +++ > b/chrome/browser/extensions/api/automation_internal/automation_internal_api.cc > @@ -247,6 +247,7 @@ class AutomationWebContentsObserver > int tree_id = AXTreeIDRegistry::GetInstance()->GetOrCreateAXTreeID( > render_frame_host->GetProcess()->GetID(), > render_frame_host->GetRoutingID()); > + LOG(ERROR) << "AXMEM Browser DestroyAccessibilityTree " << tree_id; > AXTreeIDRegistry::GetInstance()->RemoveAXTreeID(tree_id); > AutomationEventRouter::GetInstance()->DispatchTreeDestroyedEvent( > tree_id, > @@ -309,6 +310,7 @@ AutomationInternalEnableTabFunction::Run() { > > // This gets removed when the extension process dies. > AutomationEventRouter::GetInstance()->RegisterListenerForOneTree( > + extension_id(), > source_process_id(), > params->args.routing_id, > ax_tree_id); > @@ -424,6 +426,7 @@ AutomationInternalEnableDesktopFunction::Run() { > > // This gets removed when the extension process dies. > > AutomationEventRouter::GetInstance()->RegisterListenerWithDesktopPermission( > + extension_id(), > source_process_id(), > params->routing_id); > > Index: chrome/renderer/extensions/automation_internal_custom_bindings.cc > diff --git > a/chrome/renderer/extensions/automation_internal_custom_bindings.cc > b/chrome/renderer/extensions/automation_internal_custom_bindings.cc > index > 55543d3e703b68aab93f0a8e18f4d52444af4106..8f1e6d713994cc9d2b29f10691137e6dfaa58ee4 > 100644 > --- a/chrome/renderer/extensions/automation_internal_custom_bindings.cc > +++ b/chrome/renderer/extensions/automation_internal_custom_bindings.cc > @@ -199,6 +199,7 @@ void > AutomationInternalCustomBindings::DestroyAccessibilityTree( > } > > int tree_id = args[0]->Int32Value(); > + LOG(ERROR) << "AXMEM Renderer DestroyAccessibilityTree " << tree_id; > auto iter = tree_id_to_tree_cache_map_.find(tree_id); > if (iter == tree_id_to_tree_cache_map_.end()) > return; > @@ -523,6 +524,7 @@ void > AutomationInternalCustomBindings::OnAccessibilityEvent( > TreeCache* cache; > auto iter = tree_id_to_tree_cache_map_.find(tree_id); > if (iter == tree_id_to_tree_cache_map_.end()) { > + LOG(ERROR) << "AXMEM Renderer creating tree id " << params.tree_id; > cache = new TreeCache(); > cache->tab_id = -1; > cache->tree_id = params.tree_id; -- Foo X. Bar http://www.example.com To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
> > What if we just resent the entire tree whenever the active profile > changes (to all listeners in that profile)? > That'd be more complicated because we don't currently cache the trees in the browser process. Maybe we should. > > > > > Description: > > Dispatch automation events to the last used profile. > > > > BUG=515538 > > > > Please review this at https://codereview.chromium.org/1268163002/ > > > > Base URL: https://chromium.googlesource.com/chromium/src.git@master > > > > Affected files (+46, -6 lines): > > M > > > chrome/browser/extensions/api/automation_internal/automation_event_router.h > > M > > > chrome/browser/extensions/api/automation_internal/automation_event_router.cc > > M > > > chrome/browser/extensions/api/automation_internal/automation_internal_api.cc > > M chrome/renderer/extensions/automation_internal_custom_bindings.cc > > > > > > Index: > > > chrome/browser/extensions/api/automation_internal/automation_event_router.cc > > diff --git > > > a/chrome/browser/extensions/api/automation_internal/automation_event_router.cc > > > b/chrome/browser/extensions/api/automation_internal/automation_event_router.cc > > index > > > 7be3332d2e80e193420d070684884a7a387a7097..8b1120305fab852144bd3e41f25e2948d08b2b39 > > 100644 > > --- > > > a/chrome/browser/extensions/api/automation_internal/automation_event_router.cc > > +++ > > > b/chrome/browser/extensions/api/automation_internal/automation_event_router.cc > > @@ -11,6 +11,7 @@ > > #include "base/stl_util.h" > > #include "base/values.h" > > #include "chrome/browser/accessibility/ax_tree_id_registry.h" > > +#include "chrome/browser/profiles/profile_manager.h" > > #include "chrome/common/extensions/api/automation_internal.h" > > #include "chrome/common/extensions/chrome_extension_messages.h" > > #include "content/public/browser/notification_service.h" > > @@ -40,30 +41,58 @@ AutomationEventRouter::~AutomationEventRouter() { > > } > > > > void AutomationEventRouter::RegisterListenerForOneTree( > > + const ExtensionId& extension_id, > > int listener_process_id, > > int listener_routing_id, > > int source_ax_tree_id) { > > - Register(listener_process_id, listener_routing_id, source_ax_tree_id, > > false); > > + Register(extension_id, > > + listener_process_id, > > + listener_routing_id, > > + source_ax_tree_id, > > + false); > > } > > > > void AutomationEventRouter::RegisterListenerWithDesktopPermission( > > + const ExtensionId& extension_id, > > int listener_process_id, > > int listener_routing_id) { > > - Register(listener_process_id, listener_routing_id, 0, true); > > + Register(extension_id, > > + listener_process_id, > > + listener_routing_id, > > + 0, > > + true); > > } > > > > void AutomationEventRouter::DispatchAccessibilityEvent( > > const ExtensionMsg_AccessibilityEventParams& params) { > > + // Build up a map from extension id to the listener where we > > + // should send the event. > > + std::map<ExtensionId, const AutomationListener*> listeners; > > + Profile* last_used_profile = ProfileManager::GetLastUsedProfile(); > > for (const auto& listener : listeners_) { > > + // Reject listeners that don't want to listen to this tree. > > if (!listener.desktop && > > listener.tree_ids.find(params.tree_id) == > listener.tree_ids.end()) > > { > > continue; > > } > > > > + // Choose at most one listener per extension ID, > > + // favoring the last used profile. > > content::RenderProcessHost* rph = > > content::RenderProcessHost::FromID(listener.process_id); > > - rph->Send(new ExtensionMsg_AccessibilityEvent(listener.routing_id, > > - params)); > > + bool is_current_profile = (rph->GetBrowserContext() == > > last_used_profile); > > + auto iter = listeners.find(listener.extension_id); > > + if (iter == listeners.end() || is_current_profile) > > + listeners[listener.extension_id] = &listener; > > + } > > + > > + // Send the event to the selected listeners. > > + for (auto iter : listeners) { > > + const AutomationListener* listener = iter.second; > > + content::RenderProcessHost* rph = > > + content::RenderProcessHost::FromID(listener->process_id); > > + rph->Send(new ExtensionMsg_AccessibilityEvent(listener->routing_id, > > + params)); > > } > > } > > > > @@ -87,6 +116,7 @@ > > AutomationEventRouter::AutomationListener::~AutomationListener() { > > } > > > > void AutomationEventRouter::Register( > > + const ExtensionId& extension_id, > > int listener_process_id, > > int listener_routing_id, > > int ax_tree_id, > > @@ -103,6 +133,7 @@ void AutomationEventRouter::Register( > > // Add a new entry if we don't have one with that process and > routing id. > > if (iter == listeners_.end()) { > > AutomationListener listener; > > + listener.extension_id = extension_id; > > listener.routing_id = listener_routing_id; > > listener.process_id = listener_process_id; > > listener.desktop = desktop; > > Index: > > > chrome/browser/extensions/api/automation_internal/automation_event_router.h > > diff --git > > > a/chrome/browser/extensions/api/automation_internal/automation_event_router.h > > > b/chrome/browser/extensions/api/automation_internal/automation_event_router.h > > index > > > 1389b43205944e6b76b9c728074c2a54b3bd1f41..97a1307b5e645b0c2dc72c2eae3bf5895ec83aa2 > > 100644 > > --- > > > a/chrome/browser/extensions/api/automation_internal/automation_event_router.h > > +++ > > > b/chrome/browser/extensions/api/automation_internal/automation_event_router.h > > @@ -32,14 +32,16 @@ class AutomationEventRouter : public > > content::NotificationObserver { > > // wants to receive automation events from the accessibility tree > > indicated > > // by |source_ax_tree_id|. Automation events are forwarded from now > on > > // until the listener process dies. > > - void RegisterListenerForOneTree(int listener_process_id, > > + void RegisterListenerForOneTree(const ExtensionId& extension_id, > > + int listener_process_id, > > int listener_routing_id, > > int source_ax_tree_id); > > > > // Indicates that the listener at |listener_process_id|, | > > listener_routing_id| > > // wants to receive automation events from all accessibility trees > > because > > // it has Desktop permission. > > - void RegisterListenerWithDesktopPermission(int listener_process_id, > > + void RegisterListenerWithDesktopPermission(const ExtensionId& > > extension_id, > > + int listener_process_id, > > int listener_routing_id); > > > > void DispatchAccessibilityEvent( > > @@ -56,6 +58,7 @@ class AutomationEventRouter : public > > content::NotificationObserver { > > AutomationListener(); > > ~AutomationListener(); > > > > + ExtensionId extension_id; > > int routing_id; > > int process_id; > > bool desktop; > > @@ -66,6 +69,7 @@ class AutomationEventRouter : public > > content::NotificationObserver { > > ~AutomationEventRouter() override; > > > > void Register( > > + const ExtensionId& extension_id, > > int listener_process_id, > > int listener_routing_id, > > int source_ax_tree_id, > > Index: > > > chrome/browser/extensions/api/automation_internal/automation_internal_api.cc > > diff --git > > > a/chrome/browser/extensions/api/automation_internal/automation_internal_api.cc > > > b/chrome/browser/extensions/api/automation_internal/automation_internal_api.cc > > index > > > 58efba5f7592f7e215f84d71488abb3f54384211..90b26b86ffad32827345852d266a81678efd80c1 > > 100644 > > --- > > > a/chrome/browser/extensions/api/automation_internal/automation_internal_api.cc > > +++ > > > b/chrome/browser/extensions/api/automation_internal/automation_internal_api.cc > > @@ -247,6 +247,7 @@ class AutomationWebContentsObserver > > int tree_id = AXTreeIDRegistry::GetInstance()->GetOrCreateAXTreeID( > > render_frame_host->GetProcess()->GetID(), > > render_frame_host->GetRoutingID()); > > + LOG(ERROR) << "AXMEM Browser DestroyAccessibilityTree " << tree_id; > > AXTreeIDRegistry::GetInstance()->RemoveAXTreeID(tree_id); > > AutomationEventRouter::GetInstance()->DispatchTreeDestroyedEvent( > > tree_id, > > @@ -309,6 +310,7 @@ AutomationInternalEnableTabFunction::Run() { > > > > // This gets removed when the extension process dies. > > AutomationEventRouter::GetInstance()->RegisterListenerForOneTree( > > + extension_id(), > > source_process_id(), > > params->args.routing_id, > > ax_tree_id); > > @@ -424,6 +426,7 @@ AutomationInternalEnableDesktopFunction::Run() { > > > > // This gets removed when the extension process dies. > > > > > AutomationEventRouter::GetInstance()->RegisterListenerWithDesktopPermission( > > + extension_id(), > > source_process_id(), > > params->routing_id); > > > > Index: chrome/renderer/extensions/automation_internal_custom_bindings.cc > > diff --git > > a/chrome/renderer/extensions/automation_internal_custom_bindings.cc > > b/chrome/renderer/extensions/automation_internal_custom_bindings.cc > > index > > > 55543d3e703b68aab93f0a8e18f4d52444af4106..8f1e6d713994cc9d2b29f10691137e6dfaa58ee4 > > 100644 > > --- a/chrome/renderer/extensions/automation_internal_custom_bindings.cc > > +++ b/chrome/renderer/extensions/automation_internal_custom_bindings.cc > > @@ -199,6 +199,7 @@ void > > AutomationInternalCustomBindings::DestroyAccessibilityTree( > > } > > > > int tree_id = args[0]->Int32Value(); > > + LOG(ERROR) << "AXMEM Renderer DestroyAccessibilityTree " << tree_id; > > auto iter = tree_id_to_tree_cache_map_.find(tree_id); > > if (iter == tree_id_to_tree_cache_map_.end()) > > return; > > @@ -523,6 +524,7 @@ void > > AutomationInternalCustomBindings::OnAccessibilityEvent( > > TreeCache* cache; > > auto iter = tree_id_to_tree_cache_map_.find(tree_id); > > if (iter == tree_id_to_tree_cache_map_.end()) { > > + LOG(ERROR) << "AXMEM Renderer creating tree id " << params.tree_id; > > cache = new TreeCache(); > > cache->tab_id = -1; > > cache->tree_id = params.tree_id; > > > -- > Foo X. Bar > http://www.example.com > > To unsubscribe from this group and stop receiving emails from it, send an > email to chromium-reviews+unsubscribe@chromium.org. > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
OK, here's a more comprehensive solution. I tested it with multi-profile on Chrome OS and it works correctly there.
https://codereview.chromium.org/1268163002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/automation_internal/automation_event_router.cc (right): https://codereview.chromium.org/1268163002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/automation_internal/automation_event_router.cc:67: 0, nit: /* desktop tree id */ https://codereview.chromium.org/1268163002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/automation_internal/automation_event_router.cc:74: // Reject listeners that don't want to listen to this tree. nit: Skip? https://codereview.chromium.org/1268163002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/automation_internal/automation_event_router.cc:130: listener.is_active_profile = true; Does this default make sense? https://codereview.chromium.org/1268163002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/automation_internal/automation_event_router.cc:174: #if defined(OS_CHROMEOS) Can this be USE_ASH instead? https://codereview.chromium.org/1268163002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/automation_internal/automation_event_router.cc:182: listener.is_active_profile = (extension_id_count == 1 || Does this condition make sense? If I understand correctly, whenever an extension is only loaded into one profile, it is considered active even if the profile isn't currently active? https://codereview.chromium.org/1268163002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/automation_internal/automation_event_router.h (right): https://codereview.chromium.org/1268163002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/automation_internal/automation_event_router.h:35: #if defined(OS_CHROMEOS) USE_ASH? https://codereview.chromium.org/1268163002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/automation_internal/automation_event_router.h:98: void UpdateActiveProfile(); nit: comment?
https://codereview.chromium.org/1268163002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/automation_internal/automation_event_router.cc (right): https://codereview.chromium.org/1268163002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/automation_internal/automation_event_router.cc:67: 0, On 2015/08/05 22:24:36, David Tseng wrote: > nit: /* desktop tree id */ Done. https://codereview.chromium.org/1268163002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/automation_internal/automation_event_router.cc:74: // Reject listeners that don't want to listen to this tree. On 2015/08/05 22:24:36, David Tseng wrote: > nit: Skip? Done. https://codereview.chromium.org/1268163002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/automation_internal/automation_event_router.cc:130: listener.is_active_profile = true; On 2015/08/05 22:24:36, David Tseng wrote: > Does this default make sense? Deleted because we just call UpdateActiveProfile, below. https://codereview.chromium.org/1268163002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/automation_internal/automation_event_router.cc:174: #if defined(OS_CHROMEOS) On 2015/08/05 22:24:36, David Tseng wrote: > Can this be USE_ASH instead? No, because that'd be true on Windows, which uses ash for metro mode. From an include-file perspective, we want Ash, from from a semantics/behavior perspective, we want Chrome OS, which implies we have Ash available. https://codereview.chromium.org/1268163002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/automation_internal/automation_event_router.cc:182: listener.is_active_profile = (extension_id_count == 1 || On 2015/08/05 22:24:36, David Tseng wrote: > Does this condition make sense? If I understand correctly, whenever an extension > is only loaded into one profile, it is considered active even if the profile > isn't currently active? Yes, that's intentional. The idea is only to prevent multiple copies of the extension in different profiles from interfering with each other. I added a comment to clarify. https://codereview.chromium.org/1268163002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/automation_internal/automation_event_router.h (right): https://codereview.chromium.org/1268163002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/automation_internal/automation_event_router.h:98: void UpdateActiveProfile(); On 2015/08/05 22:24:36, David Tseng wrote: > nit: comment? Done.
Friendly ping
lgtm
The CQ bit was checked by dmazzoni@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1268163002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1268163002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
dmazzoni@chromium.org changed reviewers: + kalman@chromium.org
+kalman
Is there some feedback you want from me? Or just a lgtm? Because I can't give the latter, you need an IPC file owner for the messages file, and y'all should already be owners of the automation files.
I believe we need your owners approval for chrome/renderer/extensions. Would it make sense to move automation_internal_custom_bindings.cc to a subdirectory, or to add us to the OWNERS file using per-file rules?
On 2015/08/07 20:50:28, dmazzoni wrote: > I believe we need your owners approval for chrome/renderer/extensions. > > Would it make sense to move automation_internal_custom_bindings.cc to a > subdirectory, or to add us to the OWNERS file using per-file rules? Oh right. You might as well add yourselves as per-file to automation internal bindings, simplest way to do this.
I added the OWNERS file changes to this CL. Thanks.
dmazzoni@chromium.org changed reviewers: + dcheng@chromium.org
+dcheng for chrome/common/extensions/chrome_extension_messages.h
ipc changes lgtm https://codereview.chromium.org/1268163002/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/api/automation_internal/automation_event_router.cc (right): https://codereview.chromium.org/1268163002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/automation_internal/automation_event_router.cc:175: for (auto listener2 : listeners_) { const auto&
https://codereview.chromium.org/1268163002/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/api/automation_internal/automation_event_router.cc (right): https://codereview.chromium.org/1268163002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/automation_internal/automation_event_router.cc:175: for (auto listener2 : listeners_) { On 2015/08/07 21:36:09, dcheng wrote: > const auto& Done.
The CQ bit was checked by dmazzoni@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtseng@chromium.org, kalman@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/1268163002/#ps200001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1268163002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1268163002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dmazzoni@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1268163002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1268163002/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/36eee6c86d20119bab09d7cbe3ab42622a794531 Cr-Commit-Position: refs/heads/master@{#348570}
Message was sent while issue was closed.
This cl breaks ChromeVox Next and ChromeVox commands in the shelf. - chrome --login-manager - sign in - navigate in system tray with tab *- navigate in system tray with ChromeVox keys - search+shift+o, o - type "next" *- navigate in web contents result: no spoken feedback (with *- steps) This is due to automation identifying the wrong rph as the "active" ChromeVox. One ChromeVox is running in compat mode (the one using the signed in profile, and the other as force_next with the user profile). https://codereview.chromium.org/1268163002/diff/200001/chrome/browser/extensi... File chrome/browser/extensions/api/automation_internal/automation_event_router.cc (right): https://codereview.chromium.org/1268163002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/api/automation_internal/automation_event_router.cc:168: active_profile_ = ProfileManager::GetLastUsedProfile(); This doesn't appear to work. This observer gets called whenever the active user changes and we get the last active profile at *that* time. For example, when logging in, the last active profile appears to be the signed in profile. This doesn't change even when you move focus between the shelf (views) to the web content.
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/36eee6c86d20119bab09d7cbe3ab42622a794531 Cr-Commit-Position: refs/heads/master@{#348570} |
