Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2212863002:
Factor the extension loading code out of AccessibilityManager (Closed)
|
Created:
4 years, 4 months ago by dmazzoni Modified:
4 years, 3 months ago CC:
aboxhall+watch_chromium.org, chromium-apps-reviews_chromium.org, chromium-reviews, davemoore+watch_chromium.org, dmazzoni+watch_chromium.org, Dirk Pranke, dtseng+watch_chromium.org, extensions-reviews_chromium.org, je_julie, nektar+watch_chromium.org, oshima+watch_chromium.org, yuzo+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@no_load_chromevox Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFactor the extension loading code out of AccessibilityManager
Create a new class, AccessibilityExtensionLoader, and pull all of the code
that currently loads ChromeVox into it.
This is meant to be a pure refactoring change with no user-perceivable
change in functionality. If any change in behavior traces back to this
change, that's a bug.
The purpose of this change is so that AccessibilityManager can be
extended to load additional accessibility component extensions other
than ChromeVox.
BUG=634233
TBR=oshima@chromium.org
TEST=Toggle ChromeVox on login screen, logged-in screen, guest screen, and lock screen. Test logging in with ChromeVox disabled and enabled.
Committed: https://crrev.com/337c5afe3e3c93faa54c445355dc95299d0d73ef
Cr-Commit-Position: refs/heads/master@{#418401}
Patch Set 1 #Patch Set 2 : Get rid of emacs file #Patch Set 3 : Rebase on modified component loader patch #
Total comments: 5
Patch Set 4 : Rebased and updated #
Total comments: 16
Patch Set 5 : Address feedback, rename NULL to nullptr throughout #Patch Set 6 : Fix code that runs on OnUnload #
Messages
Total messages: 29 (15 generated)
The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dmazzoni@chromium.org changed reviewers: + dtseng@chromium.org
https://codereview.chromium.org/2212863002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/accessibility/accessibility_extension_loader.cc (right): https://codereview.chromium.org/2212863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/accessibility_extension_loader.cc:297: extension_service->component_loader()->AddComponentWithGuestManifest( Are you going to move this out of component loader? https://codereview.chromium.org/2212863002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/accessibility/accessibility_extension_loader.h (right): https://codereview.chromium.org/2212863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/accessibility_extension_loader.h:21: class AccessibilityExtensionLoader { Please include a small description of the class. https://codereview.chromium.org/2212863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/accessibility_extension_loader.h:31: void Unload(); I'm not entirely sure about the use of SetProfile here and the various places where profile gets used as a param.
https://codereview.chromium.org/2212863002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/accessibility/accessibility_extension_loader.cc (right): https://codereview.chromium.org/2212863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/accessibility_extension_loader.cc:297: extension_service->component_loader()->AddComponentWithGuestManifest( On 2016/08/11 22:22:13, David Tseng wrote: > Are you going to move this out of component loader? Caused a little bit of confusion because I can't seem to find where the extension (and ChromeVox) now get added as a component extension.
The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run
Refactored and updated, ready for a fresh look. https://codereview.chromium.org/2212863002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/accessibility/accessibility_extension_loader.cc (right): https://codereview.chromium.org/2212863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/accessibility_extension_loader.cc:297: extension_service->component_loader()->AddComponentWithGuestManifest( On 2016/08/11 at 22:45:45, David Tseng wrote: > On 2016/08/11 22:22:13, David Tseng wrote: > > Are you going to move this out of component loader? > > Caused a little bit of confusion because I can't seem to find where the extension (and ChromeVox) now get added as a component extension. This is the place where ChromeVox or another extension gets added as a component extension. We still call ComponentLoader to add it. The difference is that now ComponentLoader doesn't need to know about AccessibilityManager.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2212863002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/accessibility/accessibility_extension_loader.cc (right): https://codereview.chromium.org/2212863002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/accessibility_extension_loader.cc:121: : profile_(nullptr), nit: indent https://codereview.chromium.org/2212863002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/accessibility_extension_loader.cc:131: profile_ = profile; This block has similarity index with AccessibilityManager::UpdateSpokenFeedbackFromPref. One change, perhaps I'm not seeing, is: - AccessibilityManager::UpdateSpokenFeedbackFromPref in this change calls AEL::Load(), but never calls AEL::SetProfile - AccessibilityManager::UpdateSpokenFeedbackFromPref used to check that ChromeVox wasn't already loaded on the profile w/ ComponentLoader, similar to the check below, but that never occurs now I think. https://codereview.chromium.org/2212863002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/accessibility_extension_loader.cc:137: // add it to this profile. nit: more room above https://codereview.chromium.org/2212863002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/accessibility/accessibility_manager.cc (right): https://codereview.chromium.org/2212863002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/accessibility_manager.cc:1078: chromevox_loader_->InjectContentScript(render_view_host); This doesn't look equivalent.
Thanks for the thorough review https://codereview.chromium.org/2212863002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/accessibility/accessibility_extension_loader.cc (right): https://codereview.chromium.org/2212863002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/accessibility_extension_loader.cc:121: : profile_(nullptr), On 2016/09/08 at 20:39:31, David Tseng wrote: > nit: indent git cl format is happy with this...where did you expect the indent to be? https://codereview.chromium.org/2212863002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/accessibility_extension_loader.cc:131: profile_ = profile; On 2016/09/08 at 20:39:31, David Tseng wrote: > This block has similarity index with AccessibilityManager::UpdateSpokenFeedbackFromPref. One change, perhaps I'm not seeing, is: > - AccessibilityManager::UpdateSpokenFeedbackFromPref in this change calls AEL::Load(), but never calls AEL::SetProfile > - AccessibilityManager::UpdateSpokenFeedbackFromPref used to check that ChromeVox wasn't already loaded on the profile w/ ComponentLoader, similar to the check below, but that never occurs now I think. Good question. There are a few scenarios. 1. If ChromeVox was disabled but it's now being enabled, UpdateSpokenFeedbackFromPref calls AEL::Load and that sets the profile. 2. If ChromeVox was enabled but the profile is changed, AccessibilityManager::SetProfile calls AEL::SetProfile, and that catches the case where it was loaded, but just not in this profile. Previously we handled that case in UpdateSpokenFeedbackFromPref, it's true - but that's just because we knew that UpdateSpokenFeedbackFromPref would be called when SetProfile is called. So I think this should cover the important cases. https://codereview.chromium.org/2212863002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/accessibility_extension_loader.cc:137: // add it to this profile. On 2016/09/08 at 20:39:31, David Tseng wrote: > nit: more room above done https://codereview.chromium.org/2212863002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/accessibility/accessibility_manager.cc (right): https://codereview.chromium.org/2212863002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/accessibility_manager.cc:1078: chromevox_loader_->InjectContentScript(render_view_host); On 2016/09/08 at 20:39:31, David Tseng wrote: > This doesn't look equivalent. You're right, but I think it was doing extra unnecessary work before. The only place this was called was from ChromeWebViewGuestDelegate, where it only called it if AccessibilityManager::IsSpokenFeedbackEnabled() returned true. So all it needs to do (and all that it ought to do given what it's named) is inject the content script. Adding the component extension again is redundant.
lgtm https://codereview.chromium.org/2212863002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/accessibility/accessibility_extension_loader.cc (right): https://codereview.chromium.org/2212863002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/accessibility_extension_loader.cc:121: : profile_(nullptr), On 2016/09/08 21:00:55, dmazzoni wrote: > On 2016/09/08 at 20:39:31, David Tseng wrote: > > nit: indent > > git cl format is happy with this...where did you expect the indent to be? +4 since it's a continuation of the previous line. https://codereview.chromium.org/2212863002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/accessibility_extension_loader.cc:131: profile_ = profile; because we knew that UpdateSpokenFeedbackFromPref would be called when SetProfile is called. UpdateSpokenFeedbackFromPref gets called by the PrefChangeRegistrar -- I haven't looked at when that triggers. In any case, if you're sure this works, then this seems fine, but if you want to call SetProfile from UpdateSpokenFeedbackFromPref to try and maintain the mechanical part of this change then apply the a change to remove it to be safe, that would probably be better. https://codereview.chromium.org/2212863002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/accessibility/accessibility_manager.cc (right): https://codereview.chromium.org/2212863002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/accessibility_manager.cc:460: Remove https://codereview.chromium.org/2212863002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/accessibility_manager.cc:1078: chromevox_loader_->InjectContentScript(render_view_host); On 2016/09/08 21:00:55, dmazzoni wrote: > On 2016/09/08 at 20:39:31, David Tseng wrote: > > This doesn't look equivalent. > > You're right, but I think it was doing extra unnecessary work > before. The only place this was called was from > ChromeWebViewGuestDelegate, where it only called it if > AccessibilityManager::IsSpokenFeedbackEnabled() returned > true. So all it needs to do (and all that it ought to do > given what it's named) is inject the content script. > Adding the component extension again is redundant. Same comment here -- if you're sure, then that's fine; otherwise, keep the mechanical nature of this change and do this chaneg (along with the other change) in a followup. Thanks!
Also, before you commit, add a TEST= line with a rundown of the ways you tested this cl. Thanks.
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 Link to the patchset: https://codereview.chromium.org/2212863002/#ps100001 (title: "Fix code that runs on OnUnload")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Factor the extension loading code out of AccessibilityManager Create a new class, AccessibilityExtensionLoader, and pull all of the code that currently loads ChromeVox into it. This is meant to be a pure refactoring change with no user-perceivable change in functionality. If any change in behavior traces back to this change, that's a bug. The purpose of this change is so that AccessibilityManager can be extended to load additional accessibility component extensions other than ChromeVox. BUG=634233 ========== to ========== Factor the extension loading code out of AccessibilityManager Create a new class, AccessibilityExtensionLoader, and pull all of the code that currently loads ChromeVox into it. This is meant to be a pure refactoring change with no user-perceivable change in functionality. If any change in behavior traces back to this change, that's a bug. The purpose of this change is so that AccessibilityManager can be extended to load additional accessibility component extensions other than ChromeVox. BUG=634233 TEST=Toggle ChromeVox on login screen, logged-in screen, guest screen, and lock screen. Test logging in with ChromeVox disabled and enabled. ==========
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== Factor the extension loading code out of AccessibilityManager Create a new class, AccessibilityExtensionLoader, and pull all of the code that currently loads ChromeVox into it. This is meant to be a pure refactoring change with no user-perceivable change in functionality. If any change in behavior traces back to this change, that's a bug. The purpose of this change is so that AccessibilityManager can be extended to load additional accessibility component extensions other than ChromeVox. BUG=634233 TEST=Toggle ChromeVox on login screen, logged-in screen, guest screen, and lock screen. Test logging in with ChromeVox disabled and enabled. ========== to ========== Factor the extension loading code out of AccessibilityManager Create a new class, AccessibilityExtensionLoader, and pull all of the code that currently loads ChromeVox into it. This is meant to be a pure refactoring change with no user-perceivable change in functionality. If any change in behavior traces back to this change, that's a bug. The purpose of this change is so that AccessibilityManager can be extended to load additional accessibility component extensions other than ChromeVox. BUG=634233 TBR=oshima@chromium.org TEST=Toggle ChromeVox on login screen, logged-in screen, guest screen, and lock screen. Test logging in with ChromeVox disabled and enabled. ==========
dmazzoni@chromium.org changed reviewers: + oshima@chromium.org
+tbr:oshima for chrome/browser/chromeos/BUILD.gn (We should probably update OWNERS to allow anyone to make trivial changes to BUILD.gn, right?) https://codereview.chromium.org/2212863002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/accessibility/accessibility_extension_loader.cc (right): https://codereview.chromium.org/2212863002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/accessibility_extension_loader.cc:121: : profile_(nullptr), On 2016/09/08 at 21:18:42, David Tseng wrote: > On 2016/09/08 21:00:55, dmazzoni wrote: > > On 2016/09/08 at 20:39:31, David Tseng wrote: > > > nit: indent > > > > git cl format is happy with this...where did you expect the indent to be? > > +4 since it's a continuation of the previous line. The colon to begin initializers for a constructor is supposed to be at +4 from the constructor definition, so this is right. It's not considered to be a continuation of the argument list. The rest of the initializers are allowed to line up with the first one. https://codereview.chromium.org/2212863002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/accessibility_extension_loader.cc:131: profile_ = profile; On 2016/09/08 at 21:18:42, David Tseng wrote: > because we knew that UpdateSpokenFeedbackFromPref would be > called when SetProfile is called. > > UpdateSpokenFeedbackFromPref gets called by the PrefChangeRegistrar -- I haven't looked at when that triggers. > > In any case, if you're sure this works, then this seems fine, but if you want to call SetProfile from UpdateSpokenFeedbackFromPref to try and maintain the mechanical part of this change then apply the a change to remove it to be safe, that would probably be better. Makes sense, I added a call to SetProfile. https://codereview.chromium.org/2212863002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/accessibility/accessibility_manager.cc (right): https://codereview.chromium.org/2212863002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/accessibility_manager.cc:460: On 2016/09/08 at 21:18:42, David Tseng wrote: > Remove Done https://codereview.chromium.org/2212863002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/accessibility_manager.cc:1078: chromevox_loader_->InjectContentScript(render_view_host); On 2016/09/08 at 21:18:42, David Tseng wrote: > On 2016/09/08 21:00:55, dmazzoni wrote: > > On 2016/09/08 at 20:39:31, David Tseng wrote: > > > This doesn't look equivalent. > > > > You're right, but I think it was doing extra unnecessary work > > before. The only place this was called was from > > ChromeWebViewGuestDelegate, where it only called it if > > AccessibilityManager::IsSpokenFeedbackEnabled() returned > > true. So all it needs to do (and all that it ought to do > > given what it's named) is inject the content script. > > Adding the component extension again is redundant. > > Same comment here -- if you're sure, then that's fine; otherwise, keep the mechanical nature of this change and do this chaneg (along with the other change) in a followup. Thanks! Makes sense. Done.
The CQ bit was checked by dmazzoni@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Factor the extension loading code out of AccessibilityManager Create a new class, AccessibilityExtensionLoader, and pull all of the code that currently loads ChromeVox into it. This is meant to be a pure refactoring change with no user-perceivable change in functionality. If any change in behavior traces back to this change, that's a bug. The purpose of this change is so that AccessibilityManager can be extended to load additional accessibility component extensions other than ChromeVox. BUG=634233 TBR=oshima@chromium.org TEST=Toggle ChromeVox on login screen, logged-in screen, guest screen, and lock screen. Test logging in with ChromeVox disabled and enabled. ========== to ========== Factor the extension loading code out of AccessibilityManager Create a new class, AccessibilityExtensionLoader, and pull all of the code that currently loads ChromeVox into it. This is meant to be a pure refactoring change with no user-perceivable change in functionality. If any change in behavior traces back to this change, that's a bug. The purpose of this change is so that AccessibilityManager can be extended to load additional accessibility component extensions other than ChromeVox. BUG=634233 TBR=oshima@chromium.org TEST=Toggle ChromeVox on login screen, logged-in screen, guest screen, and lock screen. Test logging in with ChromeVox disabled and enabled. Committed: https://crrev.com/337c5afe3e3c93faa54c445355dc95299d0d73ef Cr-Commit-Position: refs/heads/master@{#418401} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/337c5afe3e3c93faa54c445355dc95299d0d73ef Cr-Commit-Position: refs/heads/master@{#418401} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
