Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
785723002:
Add new extension APIs related to animation policy. (Closed)
|
Created:
6 years ago by je_julie(Not used) Modified:
5 years, 10 months ago CC:
aboxhall+watch_chromium.org, asvitkine+watch_chromium.org, chromium-apps-reviews_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, extensions-reviews_chromium.org, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, plundblad+watch_chromium.org, yuzo+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd new extension property related to animation policy.
The new property is accessibilityFeatures.animationPolicy.
The new field for animation policy is added to Web Preference and the new profile is added.
accessibility_features.json is not specific to chromeos any more.
When the animation policy is updated through the extension,
Web preference is updated and WebSettings from Blink is updated.
BUG=3690
Committed: https://crrev.com/7fbb5a1a69a97d9dbd3dd959e964477935fc6f2d
Cr-Commit-Position: refs/heads/master@{#315330}
Patch Set 1 #
Total comments: 4
Patch Set 2 : animationPolicy added to accessibilityFeatures #
Total comments: 9
Patch Set 3 : Remove unnecessary code #
Total comments: 5
Patch Set 4 : Update array order #Patch Set 5 : add animation_policy_prefs files #Patch Set 6 : add animation_policy_prefs #Patch Set 7 : AccessibilityNotifier #
Total comments: 14
Patch Set 8 : Update code #
Total comments: 14
Patch Set 9 : Use prefs_tab_helper. #
Total comments: 4
Patch Set 10 : Add animation_policy_prefs. #
Total comments: 4
Patch Set 11 : Update Code #
Total comments: 4
Patch Set 12 : Move RegisterAnimationPolicyPrefs to Browser_prefs #Patch Set 13 : Rebase #Patch Set 14 : Rebase #Messages
Total messages: 74 (22 generated)
je_julie.kim@samsung.com changed reviewers: + dmazzoni@chromium.org, kalman@chromium.org, mkwst@chromium.org
Hi Kalman, Dominic and Mike, This patch adds new extension API to control animation under accessibility Private. The related patch from Blink is https://codereview.chromium.org/754813002/. The sample extension is https://drive.google.com/folderview?id=0BzK6cv0DIOPGSDBKVVVCUlloZjQ&usp=sharing. I verified it with disabling blacklist check. PTAL.
Looks reasonable to me, just a few minor issues. @kalman, others - I already suggested that Julie implement this as an accessibilityPrivate extension api, please chat with me if you have any concerns about the overall approach. I'm open to making this a public API later, but we've been working on some curated accessibility extensions that are easily installable from chrome://settings/search#accessibility and this seems like a perfect fit. https://codereview.chromium.org/785723002/diff/1/chrome/common/extensions/api... File chrome/common/extensions/api/accessibility_private.json (right): https://codereview.chromium.org/785723002/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/accessibility_private.json:221: "type": "string", Make the possible values for this type explicit using "enum": "enum": ["allowed", "once", "none"] ...and the same below for setImageAnimationPolicy https://codereview.chromium.org/785723002/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/785723002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/render_view_host_impl.cc:525: prefs.animation_policy = IMAGE_ANIMATION_POLICY_ALLOWED; nit: just 2 spaces indent for an "if" block. (Line continuations are 4 spaces, those are correct.) https://codereview.chromium.org/785723002/diff/1/content/browser/web_contents... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/785723002/diff/1/content/browser/web_contents... content/browser/web_contents/web_contents_impl.h:200: void SetImageAnimationPolicy(const std::string& policy); There should be an enum defined somewhere in Chromium code for this rather than passing a string around. Use a string only for the JavaScript API, everywhere else use an enum. https://codereview.chromium.org/785723002/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/785723002/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:45696: + <int value="941" label="ACCESSIBILITY_PRIVATE_SSETIMAGEANIMATIONPOLICY"/> nit: typo: SSET -> SET
mkwst@chromium.org changed required reviewers: + kalman@chromium.org
I'm only in the extension OWNERS for documentation changes. You'll need kalman@ (or someone sufficiently like him) to sign off on the change.
Does it make sense to put this in accessibilityFeatures? https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...
Right now everything in accessibilityFeatures is chromeos-only, but aside from that I don't see any reason not to put it there. Does it seem okay to broaden it to all platforms? Is there a way to have "platforms": ["chromeos"] apply to just some of the APIs? On Mon, Dec 8, 2014 at 8:52 AM, <kalman@chromium.org> wrote: > Does it make sense to put this in accessibilityFeatures? > > https://code.google.com/p/chromium/codesearch#chromium/ > src/chrome/common/extensions/api/accessibility_features. > json&q=accessibility_features&sq=package:chromium&l=7 > > https://codereview.chromium.org/785723002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/12/08 16:56:23, dmazzoni wrote: > Right now everything in accessibilityFeatures is chromeos-only, but aside > from that I don't see any reason not to put it there. Does it seem okay to > broaden it to all platforms? Is there a way to have "platforms": > ["chromeos"] apply to just some of the APIs? Not in some principled way that I'm aware of, but you might be able to add some annotations in here: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
Thanks for review and suggestions. Now I'm trying to move animation policy to accessibility_features. @kalman, https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte... I can see accessibility_features only work on chromeos. "platforms": ["chromeos"], I can add new data to kPrefMapping[] in preference_api.cc but how can I handle it in accessibility_features.json?
On 2014/12/09 15:16:11, je_julie wrote: > Thanks for review and suggestions. > Now I'm trying to move animation policy to accessibility_features. > > @kalman, > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte... > I can see accessibility_features only work on chromeos. > "platforms": ["chromeos"], Yeah, we'd need to delete that and just hand-write the documentation about which features are only supported on ChromeOS. > > I can add new data to kPrefMapping[] in preference_api.cc but how can I handle > it in accessibility_features.json?
kalman@chromium.org changed reviewers: + tbarzic@chromium.org
+tbarzic
Hi reviewers, I tried to expose accessibilityFeatures without any platform dependency and added animationPolcy to it. PreferenceAPI has a PrefChangeRegistrar and it notify if animationPolicy is changed. And I also tried to add test case for animationPolcy in accessibility_features_apitest.cc and test_runner.js in chrome/test/data/extensions/api_test/accessibility_features/ but I realized it covers only boolean type profiles now. So I need to add handling for string type profile. if possible, I'd like to handle it with different CL. Could you take a look at the uploaded patchset? Thanks,
Nice! https://codereview.chromium.org/785723002/diff/20001/chrome/browser/chrome_co... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/785723002/diff/20001/chrome/browser/chrome_co... chrome/browser/chrome_content_browser_client.cc:2334: std::string image_animation_policy = prefs->GetString(prefs::kAnimationPolicy); Please run git cl format over this CL. https://codereview.chromium.org/785723002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/preference/preference_api.h (right): https://codereview.chromium.org/785723002/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/preference/preference_api.h:113: void UpdateAnimationPolicy(); Blank line after this. https://codereview.chromium.org/785723002/diff/20001/chrome/common/extensions... File chrome/common/extensions/api/accessibility_features.json (right): https://codereview.chromium.org/785723002/diff/20001/chrome/common/extensions... chrome/common/extensions/api/accessibility_features.json:47: "description": "Policy for animated images. The value indicates how to handle animated images on web pages. The value indicates policy, one of allowd, once and none. <code>get()</code> requires <code>accessibilityFeatures.read</code> permission. <code>set()</code> and <code>clear()</code> require <code>accessibilityFeatures.modify</code> permission.", The enum is self-documenting, so you don't need to spell out here "The value indicates policy, one of allowd, once and none" etc, just leave out that sentence. https://codereview.chromium.org/785723002/diff/20001/chrome/common/extensions... chrome/common/extensions/api/accessibility_features.json:48: "value": ["animationPolicy", {"type": "string", "enum": ["allowed", "once", "none"]}] ... but the enum is even better if you can document it. Note that I'm doing some wrapping here, and you should too, otherwise it will be an awfully long line. It will look like this: "value": [ "animationPolicy", { "type": "string", "enum": [{ "value": "allowed". "description": "Images are allowed to animate." }, { "value": "once", "description": "...", }, { "value": "none", "description": "..." }] } ], Also note that I think "never" might be a better word that "none" but I'll let dmazzoni@ comment on that. https://codereview.chromium.org/785723002/diff/20001/chrome/common/extensions... File chrome/common/extensions/chrome_extensions_client.cc (right): https://codereview.chromium.org/785723002/diff/20001/chrome/common/extensions... chrome/common/extensions/chrome_extensions_client.cc:265: IDR_EXTENSION_API_JSON_ACCESSIBILITYFEATURES); Are you sure you need this? We *should* be auto-generating this mapping. (Also shouldn't need the IDR_EXTENSION_API_JSON_ACCESSIBILITYFEATURES)
On Thu, Dec 11, 2014 at 9:48 AM, <kalman@chromium.org> wrote: > "value": [ > "animationPolicy", { > "type": "string", > "enum": [{ > "value": "allowed". > "description": "Images are allowed to animate." > }, { > "value": "once", > "description": "...", > }, { > "value": "none", > "description": "..." > }] > } > ], > > Also note that I think "never" might be a better word that "none" but > I'll let dmazzoni@ comment on that. > I'm okay with "none". "never" sounds like there are times when we'd animate and times we wouldn't. I think the concise version of these looks clear to me: animation: allowed animation: once animation: none No strong preference, though - if you still prefer "never", that's fine. > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/12/11 19:08:36, dmazzoni wrote:
> On Thu, Dec 11, 2014 at 9:48 AM, <mailto:kalman@chromium.org> wrote:
>
> > "value": [
> > "animationPolicy", {
> > "type": "string",
> > "enum": [{
> > "value": "allowed".
> > "description": "Images are allowed to animate."
> > }, {
> > "value": "once",
> > "description": "...",
> > }, {
> > "value": "none",
> > "description": "..."
> > }]
> > }
> > ],
> >
> > Also note that I think "never" might be a better word that "none" but
> > I'll let dmazzoni@ comment on that.
> >
>
> I'm okay with "none". "never" sounds like there are times when we'd animate
> and times we wouldn't. I think the concise version of these looks clear to
> me:
>
> animation: allowed
> animation: once
> animation: none
>
> No strong preference, though - if you still prefer "never", that's fine.
I don't have a strong preference either.
Incidentally, by the same argument "always" would be better than "allowed", so
that we can stick to time-based idioms (as "once" is). "always", "once", "never"
--> but that always sounds weird.
TBH I don't actually understand this feature so I'll stop offering advice now
:-)
@kalman, I see what is your point about naming but I think we'd better follow naming from blink in order to avoid confusion and blink has 'allowed', 'once' and 'none'. I updated code to follow your comment. PTAL. Thanks, https://codereview.chromium.org/785723002/diff/20001/chrome/browser/chrome_co... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/785723002/diff/20001/chrome/browser/chrome_co... chrome/browser/chrome_content_browser_client.cc:2334: std::string image_animation_policy = prefs->GetString(prefs::kAnimationPolicy); On 2014/12/11 17:48:15, kalman wrote: > Please run git cl format over this CL. I ran git cl format with new patchset. https://codereview.chromium.org/785723002/diff/20001/chrome/common/extensions... File chrome/common/extensions/api/accessibility_features.json (right): https://codereview.chromium.org/785723002/diff/20001/chrome/common/extensions... chrome/common/extensions/api/accessibility_features.json:47: "description": "Policy for animated images. The value indicates how to handle animated images on web pages. The value indicates policy, one of allowd, once and none. <code>get()</code> requires <code>accessibilityFeatures.read</code> permission. <code>set()</code> and <code>clear()</code> require <code>accessibilityFeatures.modify</code> permission.", On 2014/12/11 17:48:15, kalman wrote: > The enum is self-documenting, so you don't need to spell out here "The value > indicates policy, one of allowd, once and none" etc, just leave out that > sentence. Done https://codereview.chromium.org/785723002/diff/20001/chrome/common/extensions... chrome/common/extensions/api/accessibility_features.json:48: "value": ["animationPolicy", {"type": "string", "enum": ["allowed", "once", "none"]}] On 2014/12/11 17:48:15, kalman wrote: > ... but the enum is even better if you can document it. Note that I'm doing some > wrapping here, and you should too, otherwise it will be an awfully long line. It > will look like this: > > "value": [ > "animationPolicy", { > "type": "string", > "enum": [{ > "value": "allowed". > "description": "Images are allowed to animate." > }, { > "value": "once", > "description": "...", > }, { > "value": "none", > "description": "..." > }] > } > ], > > Also note that I think "never" might be a better word that "none" but I'll let > dmazzoni@ comment on that. I updated it. https://codereview.chromium.org/785723002/diff/20001/chrome/common/extensions... File chrome/common/extensions/chrome_extensions_client.cc (right): https://codereview.chromium.org/785723002/diff/20001/chrome/common/extensions... chrome/common/extensions/chrome_extensions_client.cc:265: IDR_EXTENSION_API_JSON_ACCESSIBILITYFEATURES); On 2014/12/11 17:48:15, kalman wrote: > Are you sure you need this? We *should* be auto-generating this mapping. > > (Also shouldn't need the IDR_EXTENSION_API_JSON_ACCESSIBILITYFEATURES) You're right. I removed it.
https://codereview.chromium.org/785723002/diff/40001/chrome/browser/extension... File chrome/browser/extensions/api/preference/preference_api.cc (right): https://codereview.chromium.org/785723002/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/preference/preference_api.cc:155: {"animationPolicy", Could you keep this in some sort of order? Up the top seems most consistent. https://codereview.chromium.org/785723002/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/preference/preference_api.cc:499: content::BrowserAccessibilityState::GetInstance()->UpdateAnimationPolicy(); Seems like this code should be in PreferenceEventRouter?
kalman, Please check my comment. I'd like to get your opinion. Thanks, https://codereview.chromium.org/785723002/diff/40001/chrome/browser/extension... File chrome/browser/extensions/api/preference/preference_api.cc (right): https://codereview.chromium.org/785723002/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/preference/preference_api.cc:155: {"animationPolicy", On 2014/12/12 17:27:58, kalman wrote: > Could you keep this in some sort of order? Up the top seems most consistent. Done. https://codereview.chromium.org/785723002/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/preference/preference_api.cc:499: content::BrowserAccessibilityState::GetInstance()->UpdateAnimationPolicy(); On 2014/12/12 17:27:58, kalman wrote: > Seems like this code should be in PreferenceEventRouter? I saw that PreferenceEventRouter is added with OnListenerAdded and OnListenerAdded is called when js content registers 'onChange.addListener(listener);'. But addListener from js content is optional. I expect that UpdateAnimationPolicy is called whenever policy values is changed. WDYT?
https://codereview.chromium.org/785723002/diff/40001/chrome/browser/extension... File chrome/browser/extensions/api/preference/preference_api.cc (right): https://codereview.chromium.org/785723002/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/preference/preference_api.cc:499: content::BrowserAccessibilityState::GetInstance()->UpdateAnimationPolicy(); On 2014/12/14 05:46:26, je_julie wrote: > On 2014/12/12 17:27:58, kalman wrote: > > Seems like this code should be in PreferenceEventRouter? > > I saw that PreferenceEventRouter is added with OnListenerAdded and > OnListenerAdded is called when js content registers > 'onChange.addListener(listener);'. > But addListener from js content is optional. > > I expect that UpdateAnimationPolicy is called whenever policy values is changed. > WDYT? Mm, ok, so the way I read this is actually that - Constructor sets up a listener for the kAnimationPolicy prefs changing - UpdateAnimationPolicy is called when that pref changes, forwarding it to the BrowserAccessibilityState. Why does that belong inside the extension API implementation?
On 2014/12/15 03:44:07, kalman wrote: > Mm, ok, so the way I read this is actually that > - Constructor sets up a listener for the kAnimationPolicy prefs changing > - UpdateAnimationPolicy is called when that pref changes, forwarding it to the > BrowserAccessibilityState. > > Why does that belong inside the extension API implementation? @kalman, Thanks to your question, I realized that it's not proper location to put PrefChangeRegistrar. I tried to move it to BrowserAccessibilityStateImpl but I saw 'presubmit' Error because BrowserAccessibilityStateImpl is under contents and it's not allowed to include chrome/browser/profiles/profile_manager.h, chrome/common/pref_names.h and base/prefs/pref_change_registrar.h. I couldn't find any proper class to hold PrefChangeRegistrar for animation policy yet. In case of accessibility_manager, it is under chromeos. Maybe I could need new class to handle it. For this issue, I'll wait for Dominic to discuss about finding proper location to handle it. If you have any suggestion, please let me know. Thanks, @Dominic, Can I ask your advice to find good location to put PrefChangeRegistrar for animation policy?
mkwst@chromium.org changed reviewers: - mkwst@chromium.org
On 2014/12/15 06:18:46, je_julie wrote: > On 2014/12/15 03:44:07, kalman wrote: > > Mm, ok, so the way I read this is actually that > > - Constructor sets up a listener for the kAnimationPolicy prefs changing > > - UpdateAnimationPolicy is called when that pref changes, forwarding it to the > > BrowserAccessibilityState. > > > > Why does that belong inside the extension API implementation? > @kalman, > Thanks to your question, I realized that it's not proper location to put > PrefChangeRegistrar. > I tried to move it to BrowserAccessibilityStateImpl but I saw 'presubmit' Error > because > BrowserAccessibilityStateImpl is under contents and it's not allowed to include > chrome/browser/profiles/profile_manager.h, chrome/common/pref_names.h > and base/prefs/pref_change_registrar.h. > I couldn't find any proper class to hold PrefChangeRegistrar for animation > policy yet. > In case of accessibility_manager, it is under chromeos. > Maybe I could need new class to handle it. > For this issue, I'll wait for Dominic to discuss about finding proper location > to handle it. > If you have any suggestion, please let me know. > Thanks, > > @Dominic, > Can I ask your advice to find good location to put PrefChangeRegistrar for > animation policy? How about you add chrome/browser/accessibility/animation_policy_prefs.[cc,h] that's similar to invert_bubble_prefs.[cc,h] - you could register it from chrome/browser/prefs/browser_prefs.cc.
Patchset #5 (id:80001) has been deleted
Patchset #5 (id:100001) has been deleted
Patchset #5 (id:120001) has been deleted
On 2014/12/17 17:17:29, dmazzoni wrote: > > @Dominic, > > Can I ask your advice to find good location to put PrefChangeRegistrar for > > animation policy? > > How about you add chrome/browser/accessibility/animation_policy_prefs.[cc,h] > that's similar to invert_bubble_prefs.[cc,h] - you could register it from > chrome/browser/prefs/browser_prefs.cc. Hi Dominic, I added animation_policy_prefs.cc/h files and RegisterStringPref for animation policy. It's called at browser_prefs.cc like other prefs. The most concerned part is where I can add pref_change_registrar_.Add(prefs::kAnimationPolicy...) in order to get notification when the animation policy is updated. I tried to make a static class to handle it and call it after ProfileImpl::OnPrefsLoaded(). But I realized ProfileImpl::OnPrefsLoaded() could be called several times with different profles. So, it was not proper to handle it with the static class. On this patchset, I added it at profile_impl.cc. Could you take a look whether it's proper? Thanks.
On 2014/12/28 05:37:11, je_julie wrote: > The most concerned part is where I can add > pref_change_registrar_.Add(prefs::kAnimationPolicy...) in order to get > notification when the animation policy is updated. Can you add it to the BrowserAccessibilityStateImpl constructor? That might be a better place.
Patchset #9 (id:220001) has been deleted
Patchset #8 (id:200001) has been deleted
Patchset #7 (id:180001) has been deleted
On 2015/01/12 07:39:44, dmazzoni wrote: > On 2014/12/28 05:37:11, je_julie wrote: > > The most concerned part is where I can add > > pref_change_registrar_.Add(prefs::kAnimationPolicy...) in order to get > > notification when the animation policy is updated. > > Can you add it to the BrowserAccessibilityStateImpl constructor? That might be a > better place. Hi Dominic, As you can see from #21, I tried to add BrowserAccessibilityStateImpl but it's not allowed to include chrome path files in content level. So, I needed somewhere to add it from chrome level. After I found https://groups.google.com/a/chromium.org/d/msg/chromium-dev/cGuIaiOljy4/jMbYc..., I created AccessibilityNotifier as a KeyedService for accessibility. While I looking for some location to register profile for accessibility, I realized that there is no proper location without CHROMEOS. For now AccessibilityNotifier has just one pref but we can add more pref here if we need. Additionally, I modified permission_message_combinations_unittest.cc because Accessibility setting is not depedent on CRHOMEOS anymore with this patch. PTAL.
lgtm Thanks for your patience. AccessibilityNotifier looks great, I'm happy with that solution and I think it's in pretty good shape overall. Ready for @kalman to have another look. https://codereview.chromium.org/785723002/diff/240001/chrome/browser/accessib... File chrome/browser/accessibility/animation_policy_prefs.cc (right): https://codereview.chromium.org/785723002/diff/240001/chrome/browser/accessib... chrome/browser/accessibility/animation_policy_prefs.cc:13: registry->RegisterStringPref(prefs::kAnimationPolicy, "allowed", Can you use a string constant instead of "allowed"? https://codereview.chromium.org/785723002/diff/240001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/785723002/diff/240001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:2277: if (image_animation_policy == "once") Same here, these strings should be string constants, or it should be an enum rather than a string. https://codereview.chromium.org/785723002/diff/240001/chrome/browser/extensio... File chrome/browser/extensions/api/preference/preference_api.cc (right): https://codereview.chromium.org/785723002/diff/240001/chrome/browser/extensio... chrome/browser/extensions/api/preference/preference_api.cc:72: {"alternateErrorPagesEnabled", Thanks for sorting and reformatting these - but I think I'd be happier if you split that into a separate change to land independently of this one. The big worry is that if someone landed a small change to this code between when you first wrote your patch and when you land it, it'd be easy to accidentally overlook when merging. https://codereview.chromium.org/785723002/diff/240001/chrome/browser/prefs/br... File chrome/browser/prefs/browser_prefs.cc (right): https://codereview.chromium.org/785723002/diff/240001/chrome/browser/prefs/br... chrome/browser/prefs/browser_prefs.cc:415: RegisterBrowserUserPrefs(registry); I'd put RegisterAnimationPolicyPrefs(registry) here, before this line. https://codereview.chromium.org/785723002/diff/240001/chrome/browser/prefs/br... chrome/browser/prefs/browser_prefs.cc:528: RegisterAnimationPolicyPrefs(registry); This should move up https://codereview.chromium.org/785723002/diff/240001/content/browser/rendere... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/785723002/diff/240001/content/browser/rendere... content/browser/renderer_host/render_view_host_impl.cc:27: #include "content/browser/accessibility/browser_accessibility_state_impl.h" Do you still need this?
https://codereview.chromium.org/785723002/diff/240001/chrome/browser/extensio... File chrome/browser/extensions/api/preference/preference_api.cc (right): https://codereview.chromium.org/785723002/diff/240001/chrome/browser/extensio... chrome/browser/extensions/api/preference/preference_api.cc:72: {"alternateErrorPagesEnabled", On 2015/01/27 06:52:41, dmazzoni wrote: > Thanks for sorting and reformatting these - but I think I'd be happier if you > split that into a separate change to land independently of this one. > > The big worry is that if someone landed a small change to this code between when > you first wrote your patch and when you land it, it'd be easy to accidentally > overlook when merging. Yes please. If git cl format did this, then, let's figure something else out (but make sure you rebase this CL + run gclient sync in case the clang-format rules have changed; I've seen a lot of churn on enum declarations like this). https://codereview.chromium.org/785723002/diff/240001/chrome/common/extension... File chrome/common/extensions/api/accessibility_features.json (right): https://codereview.chromium.org/785723002/diff/240001/chrome/common/extension... chrome/common/extensions/api/accessibility_features.json:13: "value": ["spokenFeedback", {"type": "boolean"}] I think you will need to add annotations "ChromeOS only" to each of these (assuming that's the state of this). For example, change all instances of "Spoken feedback...permission" into "<p><strong>ChromeOS only.</strong></p>.<p>Spoken feedback...permission</p>."
Patchset #8 (id:260001) has been deleted
Patchset #8 (id:280001) has been deleted
Dominic and kalman, Thanks for a review. I updated code according to reviewers comment. BTW, I got red from 'mac_chromium_dbg_ng' because "some shards did not complete: 3, 5". I think it's not related to my change. PTAL. https://codereview.chromium.org/785723002/diff/240001/chrome/browser/accessib... File chrome/browser/accessibility/animation_policy_prefs.cc (right): https://codereview.chromium.org/785723002/diff/240001/chrome/browser/accessib... chrome/browser/accessibility/animation_policy_prefs.cc:13: registry->RegisterStringPref(prefs::kAnimationPolicy, "allowed", On 2015/01/27 06:52:41, dmazzoni wrote: > Can you use a string constant instead of "allowed"? Done. https://codereview.chromium.org/785723002/diff/240001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/785723002/diff/240001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:2277: if (image_animation_policy == "once") On 2015/01/27 06:52:41, dmazzoni wrote: > Same here, these strings should be string constants, or it should be an enum > rather than a string. I updated it with string constants. https://codereview.chromium.org/785723002/diff/240001/chrome/browser/extensio... File chrome/browser/extensions/api/preference/preference_api.cc (right): https://codereview.chromium.org/785723002/diff/240001/chrome/browser/extensio... chrome/browser/extensions/api/preference/preference_api.cc:72: {"alternateErrorPagesEnabled", On 2015/01/27 16:49:03, kalman wrote: > On 2015/01/27 06:52:41, dmazzoni wrote: > > Thanks for sorting and reformatting these - but I think I'd be happier if you > > split that into a separate change to land independently of this one. > > > > The big worry is that if someone landed a small change to this code between > when > > you first wrote your patch and when you land it, it'd be easy to accidentally > > overlook when merging. > > Yes please. > > If git cl format did this, then, let's figure something else out (but make sure > you rebase this CL + run gclient sync in case the clang-format rules have > changed; I've seen a lot of churn on enum declarations like this). I added new prefs without reformatting. As to formatting, I'll consider it on another CL. https://codereview.chromium.org/785723002/diff/240001/chrome/browser/prefs/br... File chrome/browser/prefs/browser_prefs.cc (right): https://codereview.chromium.org/785723002/diff/240001/chrome/browser/prefs/br... chrome/browser/prefs/browser_prefs.cc:415: RegisterBrowserUserPrefs(registry); On 2015/01/27 06:52:41, dmazzoni wrote: > I'd put RegisterAnimationPolicyPrefs(registry) here, before this line. You're right. I have to keep ordering. Done. https://codereview.chromium.org/785723002/diff/240001/chrome/common/extension... File chrome/common/extensions/api/accessibility_features.json (right): https://codereview.chromium.org/785723002/diff/240001/chrome/common/extension... chrome/common/extensions/api/accessibility_features.json:13: "value": ["spokenFeedback", {"type": "boolean"}] On 2015/01/27 16:49:03, kalman wrote: > I think you will need to add annotations "ChromeOS only" to each of these > (assuming that's the state of this). > > For example, change all instances of "Spoken feedback...permission" into > "<p><strong>ChromeOS only.</strong></p>.<p>Spoken feedback...permission</p>." Done. https://codereview.chromium.org/785723002/diff/240001/content/browser/rendere... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/785723002/diff/240001/content/browser/rendere... content/browser/renderer_host/render_view_host_impl.cc:27: #include "content/browser/accessibility/browser_accessibility_state_impl.h" On 2015/01/27 06:52:41, dmazzoni wrote: > Do you still need this? Sorry. I removed it.
lgtm
lgtm
dmazzoni@chromium.org changed reviewers: + jam@chromium.org, thestig@chromium.org
Ready for some OWNERS reviews: +jam for content/ +thestig for chrome/
https://codereview.chromium.org/785723002/diff/300001/chrome/browser/accessib... File chrome/browser/accessibility/accessibility_notifier.h (right): https://codereview.chromium.org/785723002/diff/300001/chrome/browser/accessib... chrome/browser/accessibility/accessibility_notifier.h:28: enum AnimationPolicy { Can't you just reuse content::ImageAnimationPolicy? https://codereview.chromium.org/785723002/diff/300001/chrome/browser/accessib... chrome/browser/accessibility/accessibility_notifier.h:35: void SetProfile(Profile*); nit: we generally include the parameter name https://codereview.chromium.org/785723002/diff/300001/chrome/browser/accessib... File chrome/browser/accessibility/animation_policy_prefs.h (right): https://codereview.chromium.org/785723002/diff/300001/chrome/browser/accessib... chrome/browser/accessibility/animation_policy_prefs.h:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. nit: no (c) https://codereview.chromium.org/785723002/diff/300001/chrome/browser/extensio... File chrome/browser/extensions/api/preference/preference_api.cc (right): https://codereview.chromium.org/785723002/diff/300001/chrome/browser/extensio... chrome/browser/extensions/api/preference/preference_api.cc:112: // is only avaiable for OS_CHROMEOS. typo https://codereview.chromium.org/785723002/diff/300001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/785723002/diff/300001/chrome/chrome_browser.g... chrome/chrome_browser.gypi:23: 'browser/accessibility/accessibility_notifier.cc', Is this needed on Android? Should all the new code be gated on ENABLE_ENTENSIONS? https://codereview.chromium.org/785723002/diff/300001/chrome/common/pref_name... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/785723002/diff/300001/chrome/common/pref_name... chrome/common/pref_names.cc:2265: const char kAnimationPolicy[] = "settings.a11y.animation_policy"; nit: have a blank line after this, like before.
i don't see why there's a new profile keyed service for these prefs, instead of just adding entries in PrefsTabHelper like how we normally watch for prefs and update renderers? https://codereview.chromium.org/785723002/diff/300001/content/public/common/w... File content/public/common/web_preferences.cc (right): https://codereview.chromium.org/785723002/diff/300001/content/public/common/w... content/public/common/web_preferences.cc:75: WebSettings::ImageAnimationPolicyAllowed); nit: indentation is off, also below https://codereview.chromium.org/785723002/diff/300001/content/public/common/w... File content/public/common/web_preferences.h (right): https://codereview.chromium.org/785723002/diff/300001/content/public/common/w... content/public/common/web_preferences.h:64: IMAGE_ANIMATION_POLICY_NO_ANIMATION documentation here would be nice https://codereview.chromium.org/785723002/diff/300001/content/public/common/w... content/public/common/web_preferences.h:203: ImageAnimationPolicy animation_policy; nit: separate this from the previous line. or just move it to 192
Thanks for OWNERS's review. @jam, Can I use PrefsTabHelper for setting animation policy? I thought PrefsTabHelper is allowed only for settings at pref tab. The animation policy is not included at pref tab yet. That's why I created a new profile keyed service after looking for proper position. If it's OK with adding animation policy to PrefsTabHelper, I'll update patch with it. WDYT? On 2015/01/28 20:29:58, jam wrote: > i don't see why there's a new profile keyed service for these prefs, instead of > just adding entries in PrefsTabHelper like how we normally watch for prefs and > update renderers?
On 2015/01/29 02:17:27, je_julie wrote: > Thanks for OWNERS's review. > > @jam, > Can I use PrefsTabHelper for setting animation policy? > I thought PrefsTabHelper is allowed only for settings at pref tab. PrefsTabHelper is used to watch changes to prefs that the renderer needs to know about. This is what this change does, but through more layers. So use PrefsTabHelper instead > The animation policy is not included at pref tab yet. > That's why I created a new profile keyed service after looking for proper > position. > If it's OK with adding animation policy to PrefsTabHelper, I'll update patch > with it. > WDYT? > > On 2015/01/28 20:29:58, jam wrote: > > i don't see why there's a new profile keyed service for these prefs, instead > of > > just adding entries in PrefsTabHelper like how we normally watch for prefs and > > update renderers?
@Lei Zhang, @jam, I updated new patchset. 1)I added #if defined(ENABLE_EXTENSIONS) at new code added. For now, animation policy can be controlled only with extension. Any time we need to expose it through others, I'll remove #if defined(ENABLE_EXTENSIONS). 2)I used PrefsTabHelper for animation policy. After I used PrefsTabHelper, I could reduce my changes. @Dominic, Because I can use PrefsTabHelper for animation policy, I think I don't need to add new files, even animation_policy_prefs. PTAL. https://codereview.chromium.org/785723002/diff/300001/chrome/browser/extensio... File chrome/browser/extensions/api/preference/preference_api.cc (right): https://codereview.chromium.org/785723002/diff/300001/chrome/browser/extensio... chrome/browser/extensions/api/preference/preference_api.cc:112: // is only avaiable for OS_CHROMEOS. On 2015/01/28 19:38:43, Lei Zhang wrote: > typo Done. https://codereview.chromium.org/785723002/diff/300001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/785723002/diff/300001/chrome/chrome_browser.g... chrome/chrome_browser.gypi:23: 'browser/accessibility/accessibility_notifier.cc', On 2015/01/28 19:38:43, Lei Zhang wrote: > Is this needed on Android? Should all the new code be gated on > ENABLE_ENTENSIONS? I removed those files because I used PrefsTabHelper and I added the guard,ENABLE_ENTENSIONS, at added code. https://codereview.chromium.org/785723002/diff/300001/chrome/common/pref_name... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/785723002/diff/300001/chrome/common/pref_name... chrome/common/pref_names.cc:2265: const char kAnimationPolicy[] = "settings.a11y.animation_policy"; On 2015/01/28 19:38:43, Lei Zhang wrote: > nit: have a blank line after this, like before. Done. https://codereview.chromium.org/785723002/diff/300001/content/public/common/w... File content/public/common/web_preferences.cc (right): https://codereview.chromium.org/785723002/diff/300001/content/public/common/w... content/public/common/web_preferences.cc:75: WebSettings::ImageAnimationPolicyAllowed); On 2015/01/28 20:29:58, jam wrote: > nit: indentation is off, also below Done. https://codereview.chromium.org/785723002/diff/300001/content/public/common/w... File content/public/common/web_preferences.h (right): https://codereview.chromium.org/785723002/diff/300001/content/public/common/w... content/public/common/web_preferences.h:64: IMAGE_ANIMATION_POLICY_NO_ANIMATION On 2015/01/28 20:29:58, jam wrote: > documentation here would be nice Done.
https://codereview.chromium.org/785723002/diff/320001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/785723002/diff/320001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:628: const char kAnimationPolicyAllowed[] = "allowed"; These probably should go in chrome/browser/accessibility https://codereview.chromium.org/785723002/diff/320001/chrome/browser/ui/prefs... File chrome/browser/ui/prefs/prefs_tab_helper.cc (right): https://codereview.chromium.org/785723002/diff/320001/chrome/browser/ui/prefs... chrome/browser/ui/prefs/prefs_tab_helper.cc:529: user_prefs::PrefRegistrySyncable::SYNCABLE_PREF); Should this be a syncable pref? I don't know the answer for sure, but the majority of the prefs below are unsyncable, so I think we should discuss this.
@Lei Zhang, Thanks for the comment. I updated the patch to follow up your comment. Could you take another look? @Dominic, Sorry for the confusion. I added animation_policy_prefs again to add const string definition and add the API for registry->RegisterStringPref. PTAL. https://codereview.chromium.org/785723002/diff/320001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/785723002/diff/320001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:628: const char kAnimationPolicyAllowed[] = "allowed"; On 2015/01/30 20:18:08, Lei Zhang wrote: > These probably should go in chrome/browser/accessibility OK. I think I'd better add animation_policy_prefs again and put const string definitions and add the API for registry->RegisterStringPref. https://codereview.chromium.org/785723002/diff/320001/chrome/browser/ui/prefs... File chrome/browser/ui/prefs/prefs_tab_helper.cc (right): https://codereview.chromium.org/785723002/diff/320001/chrome/browser/ui/prefs... chrome/browser/ui/prefs/prefs_tab_helper.cc:529: user_prefs::PrefRegistrySyncable::SYNCABLE_PREF); On 2015/01/30 20:18:08, Lei Zhang wrote: > Should this be a syncable pref? I don't know the answer for sure, but the > majority of the prefs below are unsyncable, so I think we should discuss this. There is no specific reason using SYNCABLE_PREF. I refered to kAcceptLanguages case. I think there is no problem with UNSYNCABLE_PREF for animation policy.
Patchset #10 (id:340001) has been deleted
lgtm with some nits: https://codereview.chromium.org/785723002/diff/360001/chrome/browser/accessib... File chrome/browser/accessibility/animation_policy_prefs.h (right): https://codereview.chromium.org/785723002/diff/360001/chrome/browser/accessib... chrome/browser/accessibility/animation_policy_prefs.h:12: namespace chrome { We decided to stop using the chrome namespace at some point, so please don't add more. See the "namespace policy in //chrome" thread on chromium-dev from about 1.5 years ago. https://codereview.chromium.org/785723002/diff/360001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/785723002/diff/360001/chrome/chrome_browser.g... chrome/chrome_browser.gypi:1242: 'browser/accessibility/animation_policy_prefs.cc', nit: alphabetical order: acc... before ani...
@Lei Zhang, Thanks for your comment. I updated the patchset. https://codereview.chromium.org/785723002/diff/360001/chrome/browser/accessib... File chrome/browser/accessibility/animation_policy_prefs.h (right): https://codereview.chromium.org/785723002/diff/360001/chrome/browser/accessib... chrome/browser/accessibility/animation_policy_prefs.h:12: namespace chrome { On 2015/02/03 06:47:35, Lei Zhang wrote: > We decided to stop using the chrome namespace at some point, so please don't add > more. See the "namespace policy in //chrome" thread on chromium-dev from about > 1.5 years ago. Done. https://codereview.chromium.org/785723002/diff/360001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/785723002/diff/360001/chrome/chrome_browser.g... chrome/chrome_browser.gypi:1242: 'browser/accessibility/animation_policy_prefs.cc', On 2015/02/03 06:47:35, Lei Zhang wrote: > nit: alphabetical order: acc... before ani... Done.
Patchset #11 (id:380001) has been deleted
As to trybot fail from android_dbg_tests_recipe, it seems it's flaky now. http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...
https://codereview.chromium.org/785723002/diff/400001/chrome/browser/accessib... File chrome/browser/accessibility/animation_policy_prefs.cc (right): https://codereview.chromium.org/785723002/diff/400001/chrome/browser/accessib... chrome/browser/accessibility/animation_policy_prefs.cc:14: void RegisterAnimationPolicyPrefs(user_prefs::PrefRegistrySyncable* registry) { you should call this directly from https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pre... instead of using indirection from PrefsTabHelper https://codereview.chromium.org/785723002/diff/400001/content/public/common/w... File content/public/common/web_preferences.h (right): https://codereview.chromium.org/785723002/diff/400001/content/public/common/w... content/public/common/web_preferences.h:61: #if defined(ENABLE_EXTENSIONS) extensions is a chrome concept, which content module doesn't know about. so remove the ifdefs in content. content just exposes settings and its up to embedders to set them depending on things like extensions etc...
Patchset #12 (id:420001) has been deleted
@jam, Thanks for your comment. I updated the patchset. This patchset removed '#if defined(ENABLE_EXTENSIONS)' from content, moved RegisterAnimationPolicyPrefs to Browser_prefs and rebased code on the latest code. PTAL. https://codereview.chromium.org/785723002/diff/400001/chrome/browser/accessib... File chrome/browser/accessibility/animation_policy_prefs.cc (right): https://codereview.chromium.org/785723002/diff/400001/chrome/browser/accessib... chrome/browser/accessibility/animation_policy_prefs.cc:14: void RegisterAnimationPolicyPrefs(user_prefs::PrefRegistrySyncable* registry) { On 2015/02/03 16:27:01, jam wrote: > you should call this directly from > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pre... > instead of using indirection from PrefsTabHelper Done. https://codereview.chromium.org/785723002/diff/400001/content/public/common/w... File content/public/common/web_preferences.h (right): https://codereview.chromium.org/785723002/diff/400001/content/public/common/w... content/public/common/web_preferences.h:61: #if defined(ENABLE_EXTENSIONS) On 2015/02/03 16:27:01, jam wrote: > extensions is a chrome concept, which content module doesn't know about. > > so remove the ifdefs in content. content just exposes settings and its up to > embedders to set them depending on things like extensions etc... Done.
lgtm
New patchsets have been uploaded after l-g-t-m from jam@chromium.org
On 2015/02/05 22:53:12, jam wrote: > lgtm Thanks, jam. @Dominic, I simply rebased the patchset because one of changed files was conflicted on the latest code. Now, can I try to commit this CL or do I need to get a review from more reviewers?
The rebase is fine. It looks like you need one more reviewer, for content/public/common/common_param_traits_macros.h
dmazzoni@chromium.org changed reviewers: + nasko@chromium.org
+nasko for content/public/common/*param_traits*
LGTM
The CQ bit was checked by je_julie.kim@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/785723002/460001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
New patchsets have been uploaded after l-g-t-m from nasko@chromium.org
@Dominic, I simply rebased the patchset again because of conflict with the latest code. Can I try to land it with rebased patchset?
Yes - it's okay to land after rebasing! On Feb 9, 2015 6:00 AM, <je_julie.kim@samsung.com> wrote: > @Dominic, > I simply rebased the patchset again because of conflict with the latest > code. > Can I try to land it with rebased patchset? > > https://codereview.chromium.org/785723002/ > > 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.
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/785723002/480001
Message was sent while issue was closed.
Committed patchset #14 (id:480001)
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/7fbb5a1a69a97d9dbd3dd959e964477935fc6f2d Cr-Commit-Position: refs/heads/master@{#315330} |
