Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
969813005:
Unify the Windows Parental Controls Platform Caching (Closed)
DescriptionUnify the Windows Parental Controls Platform Caching
There were two caches running around for Windows Platform Controls caching. One lived in the incognito mode prefs and the other lived in the Windows code. This change unifies both caches for easier management.
BUG=458388
TBR=rogerta
Committed: https://crrev.com/4e3ceb427b2285c6288e7daff5e2f316f19b6607
Cr-Commit-Position: refs/heads/master@{#319482}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Moved IsParentalControlActivityLoggingOn #Patch Set 3 : Downgrade the check #
Total comments: 16
Patch Set 4 : CR Feedback #Patch Set 5 : CR Feedback #
Total comments: 25
Patch Set 6 : CR Feedback #
Total comments: 1
Messages
Total messages: 42 (8 generated)
robliao@chromium.org changed reviewers: + gab@chromium.org, msw@chromium.org
robliao@chromium.org changed reviewers: - msw@chromium.org
Here's the alternate changelists with checks to the threads. The header trick won't work since the code needed to change the behavior would have linked in at production build time, too late to change for the test.
Yes, I much prefer this approach, comments below on what I think is a safer solution in less code :-) https://codereview.chromium.org/969813005/diff/1/chrome/browser/prefs/incogni... File chrome/browser/prefs/incognito_mode_prefs.cc (right): https://codereview.chromium.org/969813005/diff/1/chrome/browser/prefs/incogni... chrome/browser/prefs/incognito_mode_prefs.cc:120: // Production: The thread isn't initialized, so we're the only thread with I don't really like having the implementation rely on call order for thread-safety. How about my earlier proposal of storing this state in a: class ParentalControlsStateCache { public: ParentalControlsStateCache() : state_(base::win::IsParentalControlActivityLoggingOn()) {} bool enabled() { return state_; } private: bool state_; DISALLOW_COPY_AND_ASSIGN(ParentalControlsStateCache); } base::LazyInstance<ParentalControlsStateCache>::Leaky g_parental_controls_state_cache; Then the impl of ArePlatformParentalControlsEnabled() simply becomes: return g_parental_controls_state_cache.Get().enabled(); and initializes itself in a thread-safe manner on the first call (which can be primed from PreMainMessageLoopStart() for now and moved to another thread in a follow-up CL). https://codereview.chromium.org/969813005/diff/1/chrome/browser/prefs/incogni... chrome/browser/prefs/incognito_mode_prefs.cc:123: CHECK( DCHECK is good enough for such things. https://codereview.chromium.org/969813005/diff/1/chrome/browser/prefs/incogni... chrome/browser/prefs/incognito_mode_prefs.cc:129: base::win::IsParentalControlActivityLoggingOn() ? I would argue that we should move the implementation of this call in the unnamed namespace of this file (since it's the only user and other people using the uncached value would be incorrect and would bring back this issue) -- if multiple users require it one day (outside of chrome/), the generic impl should provide a caching solution to each individual caller anyways (but it doesn't make sense to implement this generically for now).
Actually, thinking about it some more, an idea that was brought up (by myself IIRC..) in the last meeting is that this value should barely ever change and that we should just cache it on disk and update it as a post-startup background task. Turns out prefs give us exactly the properties we need :-)! So here's what I propose: 1) Move base::win::IsParentalControlActivityLoggingOn() to the unnamed namespace of incognito_mode_prefs.cc (to avoid other people using the uncached value and bringing back this very issue). 2) Introduce a local_state boolean pref kIsParentalControlActivityLoggingOnCache. 3) Make ArePlatformParentalControlsEnabled() always rely on the pref. 4) Introduce UpdatePlatformParentalControlsCache() which is intended to be called on the UI thread and uses PostTaskAndReplyWithResult() to post a task to the BlockingPool to get value from the system and upon receiving the result back on the UI thread updates the pref (such that any future call to ArePlatformParentalControlsEnabled() in this session or the next uses the most recent value. 5) Call UpdatePlatformParentalControlsCache() late during startup (or even as an explicitly delayed task using PostDelayedTask until we have a better framework for posting "post-startup tasks"). WDYT? Cheers, Gab
On 2015/03/03 14:12:19, gab wrote: > Actually, thinking about it some more, an idea that was brought up (by myself > IIRC..) in the last meeting is that this value should barely ever change and > that we should just cache it on disk and update it as a post-startup background > task. > > Turns out prefs give us exactly the properties we need :-)! > > So here's what I propose: > > 1) Move base::win::IsParentalControlActivityLoggingOn() to the unnamed namespace > of incognito_mode_prefs.cc (to avoid other people using the uncached value and > bringing back this very issue). > > 2) Introduce a local_state boolean pref > kIsParentalControlActivityLoggingOnCache. > > 3) Make ArePlatformParentalControlsEnabled() always rely on the pref. > > 4) Introduce UpdatePlatformParentalControlsCache() which is intended to be > called on the UI thread and uses PostTaskAndReplyWithResult() to post a task to > the BlockingPool to get value from the system and upon receiving the result back > on the UI thread updates the pref (such that any future call to > ArePlatformParentalControlsEnabled() in this session or the next uses the most > recent value. > > 5) Call UpdatePlatformParentalControlsCache() late during startup (or even as an > explicitly delayed task using PostDelayedTask until we have a better framework > for posting "post-startup tasks"). > > WDYT? > > Cheers, > Gab While this value may never change, the expectation is that if the value does change it takes effect immediately, making a preference less appropriate. If we find that the value has indeed changed on the delayed update route, we would need to notify everyone who queried for this value. I'd rather save this complexity for the larger task of separating I/O from the UI thread during startup. The goal of this change is to save future callers from potentially hanging on the UI thread. It does not aim to fix the startup issue. in_reply_to: 5676830073815040 send_mail: 1 subject: Cache the Windows Parental Controls Platform Answer - Alternate
https://codereview.chromium.org/969813005/diff/1/chrome/browser/prefs/incogni... File chrome/browser/prefs/incognito_mode_prefs.cc (right): https://codereview.chromium.org/969813005/diff/1/chrome/browser/prefs/incogni... chrome/browser/prefs/incognito_mode_prefs.cc:120: // Production: The thread isn't initialized, so we're the only thread with On 2015/03/03 13:38:59, gab wrote: > I don't really like having the implementation rely on call order for > thread-safety. > > How about my earlier proposal of storing this state in a: > > class ParentalControlsStateCache { > public: > ParentalControlsStateCache() : > state_(base::win::IsParentalControlActivityLoggingOn()) {} > > bool enabled() { return state_; } > private: > bool state_; > > DISALLOW_COPY_AND_ASSIGN(ParentalControlsStateCache); > } > > base::LazyInstance<ParentalControlsStateCache>::Leaky > g_parental_controls_state_cache; > > > Then the impl of ArePlatformParentalControlsEnabled() simply becomes: > > return g_parental_controls_state_cache.Get().enabled(); > > and initializes itself in a thread-safe manner on the first call (which can be > primed from PreMainMessageLoopStart() for now and moved to another thread in a > follow-up CL). We already depend on this today. base::win::IsParentalControlActivityLoggingOn must be called from the UI or COM initialized thread. Making the state thread safe doesn't solve the problem of requiring the call to happen on a COM initialized thread. https://codereview.chromium.org/969813005/diff/1/chrome/browser/prefs/incogni... chrome/browser/prefs/incognito_mode_prefs.cc:123: CHECK( On 2015/03/03 13:38:59, gab wrote: > DCHECK is good enough for such things. If this condition fails, we are potentially presenting incognito to a user when incognito guarantees cannot be satisfied (e.g. website tracking), hence the selection for check. https://codereview.chromium.org/969813005/diff/1/chrome/browser/prefs/incogni... chrome/browser/prefs/incognito_mode_prefs.cc:129: base::win::IsParentalControlActivityLoggingOn() ? On 2015/03/03 13:39:00, gab wrote: > I would argue that we should move the implementation of this call in the unnamed > namespace of this file (since it's the only user and other people using the > uncached value would be incorrect and would bring back this issue) -- if > multiple users require it one day (outside of chrome/), the generic impl should > provide a caching solution to each individual caller anyways (but it doesn't > make sense to implement this generically for now). I'm don't quite understand this comment. The reason the implementations here were combined was for the unit tests that can't initialize.
Okay, lgtm for the post-startup improvement brought by caching, but let's keep working towards a better long-term solution. And please strongly consider moving the impl of base::win::IsParentalControlActivityLoggingOn() in this file to avoid other future callers not using the cached value by accident. https://codereview.chromium.org/969813005/diff/1/chrome/browser/prefs/incogni... File chrome/browser/prefs/incognito_mode_prefs.cc (right): https://codereview.chromium.org/969813005/diff/1/chrome/browser/prefs/incogni... chrome/browser/prefs/incognito_mode_prefs.cc:120: // Production: The thread isn't initialized, so we're the only thread with On 2015/03/03 18:10:12, robliao wrote: > On 2015/03/03 13:38:59, gab wrote: > > I don't really like having the implementation rely on call order for > > thread-safety. > > > > How about my earlier proposal of storing this state in a: > > > > class ParentalControlsStateCache { > > public: > > ParentalControlsStateCache() : > > state_(base::win::IsParentalControlActivityLoggingOn()) {} > > > > bool enabled() { return state_; } > > private: > > bool state_; > > > > DISALLOW_COPY_AND_ASSIGN(ParentalControlsStateCache); > > } > > > > base::LazyInstance<ParentalControlsStateCache>::Leaky > > g_parental_controls_state_cache; > > > > > > Then the impl of ArePlatformParentalControlsEnabled() simply becomes: > > > > return g_parental_controls_state_cache.Get().enabled(); > > > > and initializes itself in a thread-safe manner on the first call (which can be > > primed from PreMainMessageLoopStart() for now and moved to another thread in a > > follow-up CL). > > We already depend on this today. base::win::IsParentalControlActivityLoggingOn > must be called from the UI or COM initialized thread. Making the state thread > safe doesn't solve the problem of requiring the call to happen on a COM > initialized thread. Acknowledged. https://codereview.chromium.org/969813005/diff/1/chrome/browser/prefs/incogni... chrome/browser/prefs/incognito_mode_prefs.cc:123: CHECK( On 2015/03/03 18:10:12, robliao wrote: > On 2015/03/03 13:38:59, gab wrote: > > DCHECK is good enough for such things. > > If this condition fails, we are potentially presenting incognito to a user when > incognito guarantees cannot be satisfied (e.g. website tracking), hence the > selection for check. Acknowledged. https://codereview.chromium.org/969813005/diff/1/chrome/browser/prefs/incogni... chrome/browser/prefs/incognito_mode_prefs.cc:129: base::win::IsParentalControlActivityLoggingOn() ? On 2015/03/03 18:10:12, robliao wrote: > On 2015/03/03 13:39:00, gab wrote: > > I would argue that we should move the implementation of this call in the > unnamed > > namespace of this file (since it's the only user and other people using the > > uncached value would be incorrect and would bring back this issue) -- if > > multiple users require it one day (outside of chrome/), the generic impl > should > > provide a caching solution to each individual caller anyways (but it doesn't > > make sense to implement this generically for now). > > I'm don't quite understand this comment. > The reason the implementations here were combined was for the unit tests that > can't initialize. I'm talking about moving the implementation of base::win::IsParentalControlActivityLoggingOn() to the anonymous namespace of this file to avoid other accidental misuses.
https://codereview.chromium.org/969813005/diff/1/chrome/browser/prefs/incogni... File chrome/browser/prefs/incognito_mode_prefs.cc (right): https://codereview.chromium.org/969813005/diff/1/chrome/browser/prefs/incogni... chrome/browser/prefs/incognito_mode_prefs.cc:129: base::win::IsParentalControlActivityLoggingOn() ? On 2015/03/03 18:22:59, gab wrote: > On 2015/03/03 18:10:12, robliao wrote: > > On 2015/03/03 13:39:00, gab wrote: > > > I would argue that we should move the implementation of this call in the > > unnamed > > > namespace of this file (since it's the only user and other people using the > > > uncached value would be incorrect and would bring back this issue) -- if > > > multiple users require it one day (outside of chrome/), the generic impl > > should > > > provide a caching solution to each individual caller anyways (but it doesn't > > > make sense to implement this generically for now). > > > > I'm don't quite understand this comment. > > The reason the implementations here were combined was for the unit tests that > > can't initialize. > > I'm talking about moving the implementation of > base::win::IsParentalControlActivityLoggingOn() to the anonymous namespace of > this file to avoid other accidental misuses. Ah gotcha. To confirm, the proposal to move this from metro.cc to here?
Just replying to this inline below for the sake of discussion for a follow-up startup improvement (this change still lgtm). On 2015/03/03 18:01:12, robliao wrote: > On 2015/03/03 14:12:19, gab wrote: > > Actually, thinking about it some more, an idea that was brought up (by myself > > IIRC..) in the last meeting is that this value should barely ever change and > > that we should just cache it on disk and update it as a post-startup > background > > task. > > > > Turns out prefs give us exactly the properties we need :-)! > > > > So here's what I propose: > > > > 1) Move base::win::IsParentalControlActivityLoggingOn() to the unnamed > namespace > > of incognito_mode_prefs.cc (to avoid other people using the uncached value and > > bringing back this very issue). > > > > 2) Introduce a local_state boolean pref > > kIsParentalControlActivityLoggingOnCache. > > > > 3) Make ArePlatformParentalControlsEnabled() always rely on the pref. > > > > 4) Introduce UpdatePlatformParentalControlsCache() which is intended to be > > called on the UI thread and uses PostTaskAndReplyWithResult() to post a task > to > > the BlockingPool to get value from the system and upon receiving the result > back > > on the UI thread updates the pref (such that any future call to > > ArePlatformParentalControlsEnabled() in this session or the next uses the most > > recent value. > > > > 5) Call UpdatePlatformParentalControlsCache() late during startup (or even as > an > > explicitly delayed task using PostDelayedTask until we have a better framework > > for posting "post-startup tasks"). > > > > WDYT? > > > > Cheers, > > Gab > > While this value may never change, the expectation is that if the value does > change > it takes effect immediately, making a preference less appropriate. Is that truly the expectation? How often do we expect this to change in practice? I think it should happen so little that we could justify having a pref observer that just closes all incognito windows when this turns out to be |true| while the pref was |false| (in practice it would come back a few seconds after startup and is very unlikely to result in much data loss). I hadn't realized that this call needed to run on a COM initialized thread, that makes sense, is the BlockingPool COM initialized, I forget? > > If we find that the value has indeed changed on the delayed update route, we > would > need to notify everyone who queried for this value. > > I'd rather save this complexity for the larger task of separating I/O from the > UI thread during startup. > > The goal of this change is to save future callers from potentially hanging on > the > UI thread. It does not aim to fix the startup issue. > in_reply_to: 5676830073815040 > send_mail: 1 > subject: Cache the Windows Parental Controls Platform Answer - Alternate
lgtm https://codereview.chromium.org/969813005/diff/1/chrome/browser/prefs/incogni... File chrome/browser/prefs/incognito_mode_prefs.cc (right): https://codereview.chromium.org/969813005/diff/1/chrome/browser/prefs/incogni... chrome/browser/prefs/incognito_mode_prefs.cc:123: CHECK( On 2015/03/03 18:22:59, gab wrote: > On 2015/03/03 18:10:12, robliao wrote: > > On 2015/03/03 13:38:59, gab wrote: > > > DCHECK is good enough for such things. > > > > If this condition fails, we are potentially presenting incognito to a user > when > > incognito guarantees cannot be satisfied (e.g. website tracking), hence the > > selection for check. > > Acknowledged. Actually going back on this, I don't think it's possible for this check to pass in a Debug build yet fail in the wild. So while you're right that we would never want to ship a build that doesn't pass this condition and must enforce the contract here; enforcing the contract in official builds is overkill. (i.e. this not the same as CHECKs on things that affect security and could be altered by malicious third-parties who escaped the sandbox or something) https://codereview.chromium.org/969813005/diff/1/chrome/browser/prefs/incogni... chrome/browser/prefs/incognito_mode_prefs.cc:129: base::win::IsParentalControlActivityLoggingOn() ? On 2015/03/03 18:26:13, robliao wrote: > On 2015/03/03 18:22:59, gab wrote: > > On 2015/03/03 18:10:12, robliao wrote: > > > On 2015/03/03 13:39:00, gab wrote: > > > > I would argue that we should move the implementation of this call in the > > > unnamed > > > > namespace of this file (since it's the only user and other people using > the > > > > uncached value would be incorrect and would bring back this issue) -- if > > > > multiple users require it one day (outside of chrome/), the generic impl > > > should > > > > provide a caching solution to each individual caller anyways (but it > doesn't > > > > make sense to implement this generically for now). > > > > > > I'm don't quite understand this comment. > > > The reason the implementations here were combined was for the unit tests > that > > > can't initialize. > > > > I'm talking about moving the implementation of > > base::win::IsParentalControlActivityLoggingOn() to the anonymous namespace of > > this file to avoid other accidental misuses. > > Ah gotcha. To confirm, the proposal to move this from metro.cc to here? Yes.
On 2015/03/03 18:36:38, gab wrote: > lgtm Oops clicked "quick lgtm" instead of "publish mail & comments"... didn't mean to spam the lgtm's ;-) > > https://codereview.chromium.org/969813005/diff/1/chrome/browser/prefs/incogni... > File chrome/browser/prefs/incognito_mode_prefs.cc (right): > > https://codereview.chromium.org/969813005/diff/1/chrome/browser/prefs/incogni... > chrome/browser/prefs/incognito_mode_prefs.cc:123: CHECK( > On 2015/03/03 18:22:59, gab wrote: > > On 2015/03/03 18:10:12, robliao wrote: > > > On 2015/03/03 13:38:59, gab wrote: > > > > DCHECK is good enough for such things. > > > > > > If this condition fails, we are potentially presenting incognito to a user > > when > > > incognito guarantees cannot be satisfied (e.g. website tracking), hence the > > > selection for check. > > > > Acknowledged. > > Actually going back on this, I don't think it's possible for this check to pass > in a Debug build yet fail in the wild. > > So while you're right that we would never want to ship a build that doesn't pass > this condition and must enforce the contract here; enforcing the contract in > official builds is overkill. > > (i.e. this not the same as CHECKs on things that affect security and could be > altered by malicious third-parties who escaped the sandbox or something) > > https://codereview.chromium.org/969813005/diff/1/chrome/browser/prefs/incogni... > chrome/browser/prefs/incognito_mode_prefs.cc:129: > base::win::IsParentalControlActivityLoggingOn() ? > On 2015/03/03 18:26:13, robliao wrote: > > On 2015/03/03 18:22:59, gab wrote: > > > On 2015/03/03 18:10:12, robliao wrote: > > > > On 2015/03/03 13:39:00, gab wrote: > > > > > I would argue that we should move the implementation of this call in the > > > > unnamed > > > > > namespace of this file (since it's the only user and other people using > > the > > > > > uncached value would be incorrect and would bring back this issue) -- if > > > > > multiple users require it one day (outside of chrome/), the generic impl > > > > should > > > > > provide a caching solution to each individual caller anyways (but it > > doesn't > > > > > make sense to implement this generically for now). > > > > > > > > I'm don't quite understand this comment. > > > > The reason the implementations here were combined was for the unit tests > > that > > > > can't initialize. > > > > > > I'm talking about moving the implementation of > > > base::win::IsParentalControlActivityLoggingOn() to the anonymous namespace > of > > > this file to avoid other accidental misuses. > > > > Ah gotcha. To confirm, the proposal to move this from metro.cc to here? > > Yes.
Patchset #2 (id:20001) has been deleted
On 2015/03/03 18:38:13, gab wrote: > On 2015/03/03 18:36:38, gab wrote: > > lgtm > > Oops clicked "quick lgtm" instead of "publish mail & comments"... didn't mean to > spam the lgtm's ;-) > > > > > > https://codereview.chromium.org/969813005/diff/1/chrome/browser/prefs/incogni... > > File chrome/browser/prefs/incognito_mode_prefs.cc (right): > > > > > https://codereview.chromium.org/969813005/diff/1/chrome/browser/prefs/incogni... > > chrome/browser/prefs/incognito_mode_prefs.cc:123: CHECK( > > On 2015/03/03 18:22:59, gab wrote: > > > On 2015/03/03 18:10:12, robliao wrote: > > > > On 2015/03/03 13:38:59, gab wrote: > > > > > DCHECK is good enough for such things. > > > > > > > > If this condition fails, we are potentially presenting incognito to a user > > > when > > > > incognito guarantees cannot be satisfied (e.g. website tracking), hence > the > > > > selection for check. > > > > > > Acknowledged. > > > > Actually going back on this, I don't think it's possible for this check to > pass > > in a Debug build yet fail in the wild. > > > > So while you're right that we would never want to ship a build that doesn't > pass > > this condition and must enforce the contract here; enforcing the contract in > > official builds is overkill. > > > > (i.e. this not the same as CHECKs on things that affect security and could be > > altered by malicious third-parties who escaped the sandbox or something) > > > > > https://codereview.chromium.org/969813005/diff/1/chrome/browser/prefs/incogni... > > chrome/browser/prefs/incognito_mode_prefs.cc:129: > > base::win::IsParentalControlActivityLoggingOn() ? > > On 2015/03/03 18:26:13, robliao wrote: > > > On 2015/03/03 18:22:59, gab wrote: > > > > On 2015/03/03 18:10:12, robliao wrote: > > > > > On 2015/03/03 13:39:00, gab wrote: > > > > > > I would argue that we should move the implementation of this call in > the > > > > > unnamed > > > > > > namespace of this file (since it's the only user and other people > using > > > the > > > > > > uncached value would be incorrect and would bring back this issue) -- > if > > > > > > multiple users require it one day (outside of chrome/), the generic > impl > > > > > should > > > > > > provide a caching solution to each individual caller anyways (but it > > > doesn't > > > > > > make sense to implement this generically for now). > > > > > > > > > > I'm don't quite understand this comment. > > > > > The reason the implementations here were combined was for the unit tests > > > that > > > > > can't initialize. > > > > > > > > I'm talking about moving the implementation of > > > > base::win::IsParentalControlActivityLoggingOn() to the anonymous namespace > > of > > > > this file to avoid other accidental misuses. > > > > > > Ah gotcha. To confirm, the proposal to move this from metro.cc to here? > > > > Yes. I hate early returns. Turns out IsParentalControlActivityLoggingOn was also caching its result in a static. Moving it here turned out to be a good idea, allowing us to keep one cache around. As for the blocking pool, I'll have to check. In investigating this, I found that the sequenced worker pool had the capability to init com which made it a candidate for potentially doing this at startup.
On 2015/03/03 18:38:13, gab wrote: > On 2015/03/03 18:36:38, gab wrote: > > lgtm > > Oops clicked "quick lgtm" instead of "publish mail & comments"... didn't mean to > spam the lgtm's ;-) > > > > > > https://codereview.chromium.org/969813005/diff/1/chrome/browser/prefs/incogni... > > File chrome/browser/prefs/incognito_mode_prefs.cc (right): > > > > > https://codereview.chromium.org/969813005/diff/1/chrome/browser/prefs/incogni... > > chrome/browser/prefs/incognito_mode_prefs.cc:123: CHECK( > > On 2015/03/03 18:22:59, gab wrote: > > > On 2015/03/03 18:10:12, robliao wrote: > > > > On 2015/03/03 13:38:59, gab wrote: > > > > > DCHECK is good enough for such things. > > > > > > > > If this condition fails, we are potentially presenting incognito to a user > > > when > > > > incognito guarantees cannot be satisfied (e.g. website tracking), hence > the > > > > selection for check. > > > > > > Acknowledged. > > > > Actually going back on this, I don't think it's possible for this check to > pass > > in a Debug build yet fail in the wild. > > > > So while you're right that we would never want to ship a build that doesn't > pass > > this condition and must enforce the contract here; enforcing the contract in > > official builds is overkill. > > > > (i.e. this not the same as CHECKs on things that affect security and could be > > altered by malicious third-parties who escaped the sandbox or something) > > > > > https://codereview.chromium.org/969813005/diff/1/chrome/browser/prefs/incogni... > > chrome/browser/prefs/incognito_mode_prefs.cc:129: > > base::win::IsParentalControlActivityLoggingOn() ? > > On 2015/03/03 18:26:13, robliao wrote: > > > On 2015/03/03 18:22:59, gab wrote: > > > > On 2015/03/03 18:10:12, robliao wrote: > > > > > On 2015/03/03 13:39:00, gab wrote: > > > > > > I would argue that we should move the implementation of this call in > the > > > > > unnamed > > > > > > namespace of this file (since it's the only user and other people > using > > > the > > > > > > uncached value would be incorrect and would bring back this issue) -- > if > > > > > > multiple users require it one day (outside of chrome/), the generic > impl > > > > > should > > > > > > provide a caching solution to each individual caller anyways (but it > > > doesn't > > > > > > make sense to implement this generically for now). > > > > > > > > > > I'm don't quite understand this comment. > > > > > The reason the implementations here were combined was for the unit tests > > > that > > > > > can't initialize. > > > > > > > > I'm talking about moving the implementation of > > > > base::win::IsParentalControlActivityLoggingOn() to the anonymous namespace > > of > > > > this file to avoid other accidental misuses. > > > > > > Ah gotcha. To confirm, the proposal to move this from metro.cc to here? > > > > Yes. I hate early returns. Turns out IsParentalControlActivityLoggingOn was also caching its result in a static. Moving it here turned out to be a good idea, allowing us to keep one cache around. As for the blocking pool, I'll have to check. In investigating this, I found that the sequenced worker pool had the capability to init com which made it a candidate for potentially doing this at startup.
https://codereview.chromium.org/969813005/diff/1/chrome/browser/prefs/incogni... File chrome/browser/prefs/incognito_mode_prefs.cc (right): https://codereview.chromium.org/969813005/diff/1/chrome/browser/prefs/incogni... chrome/browser/prefs/incognito_mode_prefs.cc:123: CHECK( On 2015/03/03 18:36:38, gab wrote: > On 2015/03/03 18:22:59, gab wrote: > > On 2015/03/03 18:10:12, robliao wrote: > > > On 2015/03/03 13:38:59, gab wrote: > > > > DCHECK is good enough for such things. > > > > > > If this condition fails, we are potentially presenting incognito to a user > > when > > > incognito guarantees cannot be satisfied (e.g. website tracking), hence the > > > selection for check. > > > > Acknowledged. > > Actually going back on this, I don't think it's possible for this check to pass > in a Debug build yet fail in the wild. > > So while you're right that we would never want to ship a build that doesn't pass > this condition and must enforce the contract here; enforcing the contract in > official builds is overkill. > > (i.e. this not the same as CHECKs on things that affect security and could be > altered by malicious third-parties who escaped the sandbox or something) Fair enough. Downgrading the check.
On 2015/03/03 18:50:29, robliao wrote: > On 2015/03/03 18:38:13, gab wrote: > > On 2015/03/03 18:36:38, gab wrote: > > > lgtm > > > > Oops clicked "quick lgtm" instead of "publish mail & comments"... didn't mean > to > > spam the lgtm's ;-) > > > > > > > > > > > https://codereview.chromium.org/969813005/diff/1/chrome/browser/prefs/incogni... > > > File chrome/browser/prefs/incognito_mode_prefs.cc (right): > > > > > > > > > https://codereview.chromium.org/969813005/diff/1/chrome/browser/prefs/incogni... > > > chrome/browser/prefs/incognito_mode_prefs.cc:123: CHECK( > > > On 2015/03/03 18:22:59, gab wrote: > > > > On 2015/03/03 18:10:12, robliao wrote: > > > > > On 2015/03/03 13:38:59, gab wrote: > > > > > > DCHECK is good enough for such things. > > > > > > > > > > If this condition fails, we are potentially presenting incognito to a > user > > > > when > > > > > incognito guarantees cannot be satisfied (e.g. website tracking), hence > > the > > > > > selection for check. > > > > > > > > Acknowledged. > > > > > > Actually going back on this, I don't think it's possible for this check to > > pass > > > in a Debug build yet fail in the wild. > > > > > > So while you're right that we would never want to ship a build that doesn't > > pass > > > this condition and must enforce the contract here; enforcing the contract in > > > official builds is overkill. > > > > > > (i.e. this not the same as CHECKs on things that affect security and could > be > > > altered by malicious third-parties who escaped the sandbox or something) > > > > > > > > > https://codereview.chromium.org/969813005/diff/1/chrome/browser/prefs/incogni... > > > chrome/browser/prefs/incognito_mode_prefs.cc:129: > > > base::win::IsParentalControlActivityLoggingOn() ? > > > On 2015/03/03 18:26:13, robliao wrote: > > > > On 2015/03/03 18:22:59, gab wrote: > > > > > On 2015/03/03 18:10:12, robliao wrote: > > > > > > On 2015/03/03 13:39:00, gab wrote: > > > > > > > I would argue that we should move the implementation of this call in > > the > > > > > > unnamed > > > > > > > namespace of this file (since it's the only user and other people > > using > > > > the > > > > > > > uncached value would be incorrect and would bring back this issue) > -- > > if > > > > > > > multiple users require it one day (outside of chrome/), the generic > > impl > > > > > > should > > > > > > > provide a caching solution to each individual caller anyways (but it > > > > doesn't > > > > > > > make sense to implement this generically for now). > > > > > > > > > > > > I'm don't quite understand this comment. > > > > > > The reason the implementations here were combined was for the unit > tests > > > > that > > > > > > can't initialize. > > > > > > > > > > I'm talking about moving the implementation of > > > > > base::win::IsParentalControlActivityLoggingOn() to the anonymous > namespace > > > of > > > > > this file to avoid other accidental misuses. > > > > > > > > Ah gotcha. To confirm, the proposal to move this from metro.cc to here? > > > > > > Yes. > > I hate early returns. Turns out IsParentalControlActivityLoggingOn was also > caching > its result in a static. Ah ah, interesting, so this CL won't really help other than being cleaner then? > > Moving it here turned out to be a good idea, allowing us to keep one cache > around. > > As for the blocking pool, I'll have to check. In investigating this, I found > that > the sequenced worker pool had the capability to init com which made it a > candidate > for potentially doing this at startup. The BlockingPool is a SequencedWorkerPool, the main one in fact AFAIK.
On 2015/03/03 18:53:12, gab wrote: > On 2015/03/03 18:50:29, robliao wrote: > > On 2015/03/03 18:38:13, gab wrote: > > > On 2015/03/03 18:36:38, gab wrote: > > > > lgtm > > > > > > Oops clicked "quick lgtm" instead of "publish mail & comments"... didn't > mean > > to > > > spam the lgtm's ;-) > > > > > > > > > > > > > > > > > https://codereview.chromium.org/969813005/diff/1/chrome/browser/prefs/incogni... > > > > File chrome/browser/prefs/incognito_mode_prefs.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/969813005/diff/1/chrome/browser/prefs/incogni... > > > > chrome/browser/prefs/incognito_mode_prefs.cc:123: CHECK( > > > > On 2015/03/03 18:22:59, gab wrote: > > > > > On 2015/03/03 18:10:12, robliao wrote: > > > > > > On 2015/03/03 13:38:59, gab wrote: > > > > > > > DCHECK is good enough for such things. > > > > > > > > > > > > If this condition fails, we are potentially presenting incognito to a > > user > > > > > when > > > > > > incognito guarantees cannot be satisfied (e.g. website tracking), > hence > > > the > > > > > > selection for check. > > > > > > > > > > Acknowledged. > > > > > > > > Actually going back on this, I don't think it's possible for this check to > > > pass > > > > in a Debug build yet fail in the wild. > > > > > > > > So while you're right that we would never want to ship a build that > doesn't > > > pass > > > > this condition and must enforce the contract here; enforcing the contract > in > > > > official builds is overkill. > > > > > > > > (i.e. this not the same as CHECKs on things that affect security and could > > be > > > > altered by malicious third-parties who escaped the sandbox or something) > > > > > > > > > > > > > > https://codereview.chromium.org/969813005/diff/1/chrome/browser/prefs/incogni... > > > > chrome/browser/prefs/incognito_mode_prefs.cc:129: > > > > base::win::IsParentalControlActivityLoggingOn() ? > > > > On 2015/03/03 18:26:13, robliao wrote: > > > > > On 2015/03/03 18:22:59, gab wrote: > > > > > > On 2015/03/03 18:10:12, robliao wrote: > > > > > > > On 2015/03/03 13:39:00, gab wrote: > > > > > > > > I would argue that we should move the implementation of this call > in > > > the > > > > > > > unnamed > > > > > > > > namespace of this file (since it's the only user and other people > > > using > > > > > the > > > > > > > > uncached value would be incorrect and would bring back this issue) > > -- > > > if > > > > > > > > multiple users require it one day (outside of chrome/), the > generic > > > impl > > > > > > > should > > > > > > > > provide a caching solution to each individual caller anyways (but > it > > > > > doesn't > > > > > > > > make sense to implement this generically for now). > > > > > > > > > > > > > > I'm don't quite understand this comment. > > > > > > > The reason the implementations here were combined was for the unit > > tests > > > > > that > > > > > > > can't initialize. > > > > > > > > > > > > I'm talking about moving the implementation of > > > > > > base::win::IsParentalControlActivityLoggingOn() to the anonymous > > namespace > > > > of > > > > > > this file to avoid other accidental misuses. > > > > > > > > > > Ah gotcha. To confirm, the proposal to move this from metro.cc to here? > > > > > > > > Yes. > > > > I hate early returns. Turns out IsParentalControlActivityLoggingOn was also > > caching > > its result in a static. > > Ah ah, interesting, so this CL won't really help other than being cleaner then? > > > > > Moving it here turned out to be a good idea, allowing us to keep one cache > > around. > > > > As for the blocking pool, I'll have to check. In investigating this, I found > > that > > the sequenced worker pool had the capability to init com which made it a > > candidate > > for potentially doing this at startup. > > The BlockingPool is a SequencedWorkerPool, the main one in fact AFAIK. > Ah ah, interesting, so this CL won't really help other than being cleaner then? Ironically enough, that is indeed the case. Well, it was a good cleanup exercise since one of the metro folks were wondering why this code was in metro.cc in the first place.
robliao@chromium.org changed reviewers: + blundell@chromium.org, grt@chromium.org, sky@chromium.org
The saving grace is that it does help stage the inevitable cleanup here when we try to move this off of the UI thread. OWNERS, please provide owner approval for the following files. Thanks! grt: base/win/metro.h base/win/metro.cc sky: chrome/browser/chrome_browser_main_win.cc blundell: chrome/browser/signin/signin_header_helper.cc
blundell@chromium.org changed reviewers: + rogerta@chromium.org
blundell -> rogerta
https://codereview.chromium.org/969813005/diff/60001/chrome/browser/prefs/inc... File chrome/browser/prefs/incognito_mode_prefs.cc (right): https://codereview.chromium.org/969813005/diff/60001/chrome/browser/prefs/inc... chrome/browser/prefs/incognito_mode_prefs.cc:44: // feature is available on Windows Vista and beyond. Is it? You only query it for Win7+? https://codereview.chromium.org/969813005/diff/60001/chrome/browser/prefs/inc... chrome/browser/prefs/incognito_mode_prefs.cc:45: // This function should be called on a COM Initialized thread. Also mention that this can be blocking? And move the wait/IO assertions here? https://codereview.chromium.org/969813005/diff/60001/chrome/browser/prefs/inc... chrome/browser/prefs/incognito_mode_prefs.cc:47: using namespace base::win; Hmmm, I wouldn't do this, what does it simplify? I like explicit everywhere unless it feels like clutter. (and if you insist on doing it, at least get rid of base::win:: instances below ;-)!) https://codereview.chromium.org/969813005/diff/60001/chrome/browser/prefs/inc... chrome/browser/prefs/incognito_mode_prefs.cc:60: hr = parent_controls->GetUserSettings(NULL, settings.Receive()); s/NULL/nullptr
Patchset #4 (id:80001) has been deleted
https://codereview.chromium.org/969813005/diff/60001/chrome/browser/chrome_br... File chrome/browser/chrome_browser_main_win.cc (right): https://codereview.chromium.org/969813005/diff/60001/chrome/browser/chrome_br... chrome/browser/chrome_browser_main_win.cc:215: IncognitoModePrefs::ArePlatformParentalControlsEnabled(); IMO the old name is clearer. I wouldn't expect a function named with 'are' to have side effects.
https://codereview.chromium.org/969813005/diff/60001/chrome/browser/chrome_br... File chrome/browser/chrome_browser_main_win.cc (right): https://codereview.chromium.org/969813005/diff/60001/chrome/browser/chrome_br... chrome/browser/chrome_browser_main_win.cc:215: IncognitoModePrefs::ArePlatformParentalControlsEnabled(); On 2015/03/03 20:16:53, sky wrote: > IMO the old name is clearer. I wouldn't expect a function named with 'are' to > have side effects. We stuck with this as a compromise. It's either update all the test cases (https://codereview.chromium.org/950053002/) or perform the initialization with the first caller. https://codereview.chromium.org/969813005/diff/60001/chrome/browser/prefs/inc... File chrome/browser/prefs/incognito_mode_prefs.cc (right): https://codereview.chromium.org/969813005/diff/60001/chrome/browser/prefs/inc... chrome/browser/prefs/incognito_mode_prefs.cc:44: // feature is available on Windows Vista and beyond. On 2015/03/03 20:05:03, gab wrote: > Is it? You only query it for Win7+? Looks like this comment wasn't updated in https://codereview.chromium.org/671273003 Fixed. https://codereview.chromium.org/969813005/diff/60001/chrome/browser/prefs/inc... chrome/browser/prefs/incognito_mode_prefs.cc:45: // This function should be called on a COM Initialized thread. On 2015/03/03 20:05:03, gab wrote: > Also mention that this can be blocking? And move the wait/IO assertions here? Done. https://codereview.chromium.org/969813005/diff/60001/chrome/browser/prefs/inc... chrome/browser/prefs/incognito_mode_prefs.cc:47: using namespace base::win; On 2015/03/03 20:05:03, gab wrote: > Hmmm, I wouldn't do this, what does it simplify? I like explicit everywhere > unless it feels like clutter. > > (and if you insist on doing it, at least get rid of base::win:: instances below > ;-)!) This simplifies the ScopedComPtr uses below. Unfortunately, there's ambiguity for base::win::GetVersion with GetVersion from Windows, so that has to stay. https://codereview.chromium.org/969813005/diff/60001/chrome/browser/prefs/inc... chrome/browser/prefs/incognito_mode_prefs.cc:60: hr = parent_controls->GetUserSettings(NULL, settings.Receive()); On 2015/03/03 20:05:03, gab wrote: > s/NULL/nullptr Done.
If you're looking for a different thread for COM, the FILE thread is an option (the blocking pool is not). https://codereview.chromium.org/969813005/diff/60001/chrome/browser/chrome_br... File chrome/browser/chrome_browser_main_win.cc (right): https://codereview.chromium.org/969813005/diff/60001/chrome/browser/chrome_br... chrome/browser/chrome_browser_main_win.cc:214: // Initializes and caches the result on Windows. nit: // Prime the parental controls cache on Windows. https://codereview.chromium.org/969813005/diff/60001/chrome/browser/chrome_br... chrome/browser/chrome_browser_main_win.cc:215: IncognitoModePrefs::ArePlatformParentalControlsEnabled(); On 2015/03/03 20:16:53, sky wrote: > IMO the old name is clearer. I wouldn't expect a function named with 'are' to > have side effects. Seconded. I think it's more clear to have separate "InitializeThingOnUIThread" and "QueryThing" methods since their requirements differ. https://codereview.chromium.org/969813005/diff/60001/chrome/browser/prefs/inc... File chrome/browser/prefs/incognito_mode_prefs.cc (right): https://codereview.chromium.org/969813005/diff/60001/chrome/browser/prefs/inc... chrome/browser/prefs/incognito_mode_prefs.cc:149: if (g_parental_controls_state == ParentalControlsState::UNKNOWN) { g_parental_controls_state should be a local static since it isn't accessed outside of this function.
https://codereview.chromium.org/969813005/diff/60001/chrome/browser/chrome_br... File chrome/browser/chrome_browser_main_win.cc (right): https://codereview.chromium.org/969813005/diff/60001/chrome/browser/chrome_br... chrome/browser/chrome_browser_main_win.cc:214: // Initializes and caches the result on Windows. On 2015/03/03 20:28:54, grt wrote: > nit: > // Prime the parental controls cache on Windows. Done. https://codereview.chromium.org/969813005/diff/60001/chrome/browser/chrome_br... chrome/browser/chrome_browser_main_win.cc:215: IncognitoModePrefs::ArePlatformParentalControlsEnabled(); On 2015/03/03 20:28:54, grt wrote: > On 2015/03/03 20:16:53, sky wrote: > > IMO the old name is clearer. I wouldn't expect a function named with 'are' to > > have side effects. > > Seconded. I think it's more clear to have separate "InitializeThingOnUIThread" > and "QueryThing" methods since their requirements differ. Obviously I'm cool with the separation of requirements since that was the first delta (https://codereview.chromium.org/950053002/). The main concern with this approach is that it requires all test cases and future tests to call this under certain conditions. What are your thoughts on that? https://codereview.chromium.org/969813005/diff/60001/chrome/browser/prefs/inc... File chrome/browser/prefs/incognito_mode_prefs.cc (right): https://codereview.chromium.org/969813005/diff/60001/chrome/browser/prefs/inc... chrome/browser/prefs/incognito_mode_prefs.cc:149: if (g_parental_controls_state == ParentalControlsState::UNKNOWN) { On 2015/03/03 20:28:54, grt wrote: > g_parental_controls_state should be a local static since it isn't accessed > outside of this function. Done.
Ok, LGTM
https://codereview.chromium.org/969813005/diff/120001/base/win/metro.cc File base/win/metro.cc (right): https://codereview.chromium.org/969813005/diff/120001/base/win/metro.cc#newcode7 base/win/metro.cc:7: #include "base/message_loop/message_loop.h" this also seems to be unused. would you remove it too? https://codereview.chromium.org/969813005/diff/120001/base/win/metro.cc#newcode9 base/win/metro.cc:9: #include "base/win/scoped_comptr.h" unused https://codereview.chromium.org/969813005/diff/120001/base/win/metro.cc#newco... base/win/metro.cc:10: #include "base/win/windows_version.h" unused https://codereview.chromium.org/969813005/diff/120001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main_win.cc (right): https://codereview.chromium.org/969813005/diff/120001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main_win.cc:215: IncognitoModePrefs::ArePlatformParentalControlsEnabled(); ignore_result(...) to make this more explicit (and #include "base/macros.h"). https://codereview.chromium.org/969813005/diff/120001/chrome/browser/prefs/in... File chrome/browser/prefs/incognito_mode_prefs.cc (right): https://codereview.chromium.org/969813005/diff/120001/chrome/browser/prefs/in... chrome/browser/prefs/incognito_mode_prefs.cc:37: using namespace base::win; this doesn't seem to reduce the need for wrapping on lines 47 and 53. please remove and use: base::win::ScopedComPtr<IWindowsParentalControlsCore> parent_controls; and base::win::ScopedComPtr<IWPCSettings> settings; down there (unless i'm missing another place where it really does lead to savings). https://codereview.chromium.org/969813005/diff/120001/chrome/browser/prefs/in... chrome/browser/prefs/incognito_mode_prefs.cc:64: } // empty namespace while you're here, please change to: } // namespace https://codereview.chromium.org/969813005/diff/120001/chrome/browser/prefs/in... chrome/browser/prefs/incognito_mode_prefs.cc:151: } g_parental_controls_state = ParentalControlsState::UNKNOWN; no need for g_ since this is no longer a global https://codereview.chromium.org/969813005/diff/120001/chrome/browser/prefs/in... chrome/browser/prefs/incognito_mode_prefs.cc:157: !BrowserThread::IsThreadInitialized(BrowserThread::UI) || formatting nit: DCHECK(!BrowserThread::IsThreadInitialized(BrowserThread::UI) || BrowserThread::CurrentlyOn(BrowserThread::UI));
A few more nits from me on the latest version, should be good after that and Greg's comments. https://codereview.chromium.org/969813005/diff/120001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main_win.cc (right): https://codereview.chromium.org/969813005/diff/120001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main_win.cc:215: IncognitoModePrefs::ArePlatformParentalControlsEnabled(); On 2015/03/04 03:59:39, grt wrote: > ignore_result(...) to make this more explicit (and #include "base/macros.h"). That makes sense to me, also use WARN_UNUSED_RESULT on the method declaration to enforce this call site to explicitly do ignore_result(). https://codereview.chromium.org/969813005/diff/120001/chrome/browser/prefs/in... File chrome/browser/prefs/incognito_mode_prefs.cc (right): https://codereview.chromium.org/969813005/diff/120001/chrome/browser/prefs/in... chrome/browser/prefs/incognito_mode_prefs.cc:143: // Possible values for the parental controls state. Consider removing this comment, feels redundant (i.e. that's always what an enum is -- a set of possible value for the thing it's named after). https://codereview.chromium.org/969813005/diff/120001/chrome/browser/prefs/in... chrome/browser/prefs/incognito_mode_prefs.cc:144: static enum class ParentalControlsState { Move static keyword right before variable declaration (I know both mean the same thing, but I prefer reading it where it's applied (on the variable) then on the enum), e.g.: enum class ... { ... } static parental_controls_state = ...;
https://codereview.chromium.org/969813005/diff/120001/chrome/browser/prefs/in... File chrome/browser/prefs/incognito_mode_prefs.cc (right): https://codereview.chromium.org/969813005/diff/120001/chrome/browser/prefs/in... chrome/browser/prefs/incognito_mode_prefs.cc:144: static enum class ParentalControlsState { On 2015/03/04 16:25:23, gab wrote: > Move static keyword right before variable declaration (I know both mean the same > thing, but I prefer reading it where it's applied (on the variable) then on the > enum), e.g.: > > enum class ... { > ... > } static parental_controls_state = ...; Not sure I agree here. You would never write int static foo = ...; If you don't like the static at the front (I don't blame you), then I suggesting changing to: enum class Blah { ... }; static Blah blah = ...;
https://codereview.chromium.org/969813005/diff/120001/chrome/browser/prefs/in... File chrome/browser/prefs/incognito_mode_prefs.cc (right): https://codereview.chromium.org/969813005/diff/120001/chrome/browser/prefs/in... chrome/browser/prefs/incognito_mode_prefs.cc:144: static enum class ParentalControlsState { On 2015/03/04 16:45:12, grt wrote: > On 2015/03/04 16:25:23, gab wrote: > > Move static keyword right before variable declaration (I know both mean the > same > > thing, but I prefer reading it where it's applied (on the variable) then on > the > > enum), e.g.: > > > > enum class ... { > > ... > > } static parental_controls_state = ...; > > Not sure I agree here. You would never write > int static foo = ...; > If you don't like the static at the front (I don't blame you), then I suggesting > changing to: > enum class Blah { > ... > }; > static Blah blah = ...; I see your point, I still prefer my proposal though, but don't care strongly either way. I find static enum class { ... } var = FOO; hides the static keyword (despite it being in proper order as far as typical <keywords> <type> <var_name> = <value> order). Where as having an extra line for an explicit variable declaration feels overkill when you only need one variable of this type. But I guess I'm fine with anything but having the static keyword before the enum declaration which hinders readability IMO as it's so far from the variable declaration.
https://codereview.chromium.org/969813005/diff/120001/base/win/metro.cc File base/win/metro.cc (right): https://codereview.chromium.org/969813005/diff/120001/base/win/metro.cc#newcode7 base/win/metro.cc:7: #include "base/message_loop/message_loop.h" On 2015/03/04 03:59:39, grt wrote: > this also seems to be unused. would you remove it too? Done. https://codereview.chromium.org/969813005/diff/120001/base/win/metro.cc#newcode9 base/win/metro.cc:9: #include "base/win/scoped_comptr.h" On 2015/03/04 03:59:39, grt wrote: > unused Done. https://codereview.chromium.org/969813005/diff/120001/base/win/metro.cc#newco... base/win/metro.cc:10: #include "base/win/windows_version.h" On 2015/03/04 03:59:39, grt wrote: > unused Done. https://codereview.chromium.org/969813005/diff/120001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main_win.cc (right): https://codereview.chromium.org/969813005/diff/120001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main_win.cc:215: IncognitoModePrefs::ArePlatformParentalControlsEnabled(); On 2015/03/04 16:25:23, gab wrote: > On 2015/03/04 03:59:39, grt wrote: > > ignore_result(...) to make this more explicit (and #include "base/macros.h"). > > That makes sense to me, also use WARN_UNUSED_RESULT on the method declaration to > enforce this call site to explicitly do ignore_result(). Done. FYI, WARN_UNUSED_RESULT only applies to gcc. https://codereview.chromium.org/969813005/diff/120001/chrome/browser/prefs/in... File chrome/browser/prefs/incognito_mode_prefs.cc (right): https://codereview.chromium.org/969813005/diff/120001/chrome/browser/prefs/in... chrome/browser/prefs/incognito_mode_prefs.cc:37: using namespace base::win; On 2015/03/04 03:59:39, grt wrote: > this doesn't seem to reduce the need for wrapping on lines 47 and 53. please > remove and use: > base::win::ScopedComPtr<IWindowsParentalControlsCore> parent_controls; > and > base::win::ScopedComPtr<IWPCSettings> settings; > down there (unless i'm missing another place where it really does lead to > savings). More of a style thing than anything, since that's what this is. I'm used to seeing non-namespaced COM smart pointers. https://codereview.chromium.org/969813005/diff/120001/chrome/browser/prefs/in... chrome/browser/prefs/incognito_mode_prefs.cc:64: } // empty namespace On 2015/03/04 03:59:39, grt wrote: > while you're here, please change to: > } // namespace Done. https://codereview.chromium.org/969813005/diff/120001/chrome/browser/prefs/in... chrome/browser/prefs/incognito_mode_prefs.cc:143: // Possible values for the parental controls state. On 2015/03/04 16:25:23, gab wrote: > Consider removing this comment, feels redundant (i.e. that's always what an enum > is -- a set of possible value for the thing it's named after). Just following the style set by enums in the neighboring .h file. Some folks are a stickler for this. I would prefer to remove it along with the annotating comments since they don't add value. https://codereview.chromium.org/969813005/diff/120001/chrome/browser/prefs/in... chrome/browser/prefs/incognito_mode_prefs.cc:144: static enum class ParentalControlsState { On 2015/03/04 16:53:19, gab wrote: > On 2015/03/04 16:45:12, grt wrote: > > On 2015/03/04 16:25:23, gab wrote: > > > Move static keyword right before variable declaration (I know both mean the > > same > > > thing, but I prefer reading it where it's applied (on the variable) then on > > the > > > enum), e.g.: > > > > > > enum class ... { > > > ... > > > } static parental_controls_state = ...; > > > > Not sure I agree here. You would never write > > int static foo = ...; > > If you don't like the static at the front (I don't blame you), then I > suggesting > > changing to: > > enum class Blah { > > ... > > }; > > static Blah blah = ...; > > I see your point, I still prefer my proposal though, but don't care strongly > either way. > > I find > static enum class { > ... > } var = FOO; > hides the static keyword (despite it being in proper order as far as typical > <keywords> <type> <var_name> = <value> order). > > Where as having an extra line for an explicit variable declaration feels > overkill when you only need one variable of this type. > > But I guess I'm fine with anything but having the static keyword before the enum > declaration which hinders readability IMO as it's so far from the variable > declaration. A search from the earlier proposal revealed that this inline type + variable declaration isn't used very often. The C++ style guide doesn't have a ruling on either declaration order or type + variable declaration other than to be consistent. I personally find declaring a type and the variable separately more readable and more reusable should we decide to use this later as it disrupts less code. The location of the static keyword stops being an issue with separation, so I think we should go with that. https://codereview.chromium.org/969813005/diff/120001/chrome/browser/prefs/in... chrome/browser/prefs/incognito_mode_prefs.cc:151: } g_parental_controls_state = ParentalControlsState::UNKNOWN; On 2015/03/04 03:59:39, grt wrote: > no need for g_ since this is no longer a global Missed that. Thanks. https://codereview.chromium.org/969813005/diff/120001/chrome/browser/prefs/in... chrome/browser/prefs/incognito_mode_prefs.cc:157: !BrowserThread::IsThreadInitialized(BrowserThread::UI) || On 2015/03/04 03:59:39, grt wrote: > formatting nit: > DCHECK(!BrowserThread::IsThreadInitialized(BrowserThread::UI) || > BrowserThread::CurrentlyOn(BrowserThread::UI)); Done.
lgtm (once again!), w/ one comment on a comment Cheers, Gab https://codereview.chromium.org/969813005/diff/120001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main_win.cc (right): https://codereview.chromium.org/969813005/diff/120001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main_win.cc:215: IncognitoModePrefs::ArePlatformParentalControlsEnabled(); On 2015/03/04 18:47:44, robliao wrote: > On 2015/03/04 16:25:23, gab wrote: > > On 2015/03/04 03:59:39, grt wrote: > > > ignore_result(...) to make this more explicit (and #include > "base/macros.h"). > > > > That makes sense to me, also use WARN_UNUSED_RESULT on the method declaration > to > > enforce this call site to explicitly do ignore_result(). > > Done. > FYI, WARN_UNUSED_RESULT only applies to gcc. Right, at least it gives it waterfall coverage (except in win-only code, but better than nothing). https://codereview.chromium.org/969813005/diff/120001/chrome/browser/prefs/in... File chrome/browser/prefs/incognito_mode_prefs.cc (right): https://codereview.chromium.org/969813005/diff/120001/chrome/browser/prefs/in... chrome/browser/prefs/incognito_mode_prefs.cc:144: static enum class ParentalControlsState { On 2015/03/04 18:47:44, robliao wrote: > On 2015/03/04 16:53:19, gab wrote: > > On 2015/03/04 16:45:12, grt wrote: > > > On 2015/03/04 16:25:23, gab wrote: > > > > Move static keyword right before variable declaration (I know both mean > the > > > same > > > > thing, but I prefer reading it where it's applied (on the variable) then > on > > > the > > > > enum), e.g.: > > > > > > > > enum class ... { > > > > ... > > > > } static parental_controls_state = ...; > > > > > > Not sure I agree here. You would never write > > > int static foo = ...; > > > If you don't like the static at the front (I don't blame you), then I > > suggesting > > > changing to: > > > enum class Blah { > > > ... > > > }; > > > static Blah blah = ...; > > > > I see your point, I still prefer my proposal though, but don't care strongly > > either way. > > > > I find > > static enum class { > > ... > > } var = FOO; > > hides the static keyword (despite it being in proper order as far as typical > > <keywords> <type> <var_name> = <value> order). > > > > Where as having an extra line for an explicit variable declaration feels > > overkill when you only need one variable of this type. > > > > But I guess I'm fine with anything but having the static keyword before the > enum > > declaration which hinders readability IMO as it's so far from the variable > > declaration. > > A search from the earlier proposal revealed that this inline type + variable > declaration isn't used very often. > > The C++ style guide doesn't have a ruling on either declaration order or type + > variable declaration other than to be consistent. I personally find declaring a > type and the variable separately more readable and more reusable should we > decide to use this later as it disrupts less code. > > The location of the static keyword stops being an issue with separation, so I > think we should go with that. Okay, sgtm. https://codereview.chromium.org/969813005/diff/140001/chrome/browser/prefs/in... File chrome/browser/prefs/incognito_mode_prefs.h (right): https://codereview.chromium.org/969813005/diff/140001/chrome/browser/prefs/in... chrome/browser/prefs/incognito_mode_prefs.h:70: // allowed. Subsequent calls may be from any thread. Also mention that this must be called on the main thread *before* other threads are created (otherwise I think the impl is not thread-safe because a static variable isn't -- wouldn't lead to incorrect results/corruption, but could result in computing the value many times). base::LazyInstance can help us work around that once we want to move the initialization to another thread.
lgtm
On 2015/03/04 20:18:45, grt wrote: > lgtm rogerta ping for chrome/browser/signin/signin_header_helper.cc Thanks!
The CQ bit was checked by robliao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/969813005/#ps140001 (title: "CR Feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/969813005/140001
Message was sent while issue was closed.
Committed patchset #6 (id:140001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/4e3ceb427b2285c6288e7daff5e2f316f19b6607 Cr-Commit-Position: refs/heads/master@{#319482} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
