Hide "New incognito window" from jump list if disabled by policy.

Hide "New incognito window" from jump list if disabled by policy.

BUG=148392
Committed: https://crrev.com/b943d1c8690d35fc9aeaedaca39c32baddffc082
Cr-Commit-Position: refs/heads/master@{#293850}

[Joao da Silva, 2014-09-08 09:27:25]

joaodasilva@chromium.org changed reviewers:
+ sky@chromium.org

[Joao da Silva, 2014-09-08 09:27:25]

Hi Scott, PTAL. Thanks!

[sky, 2014-09-08 16:15:18]

https://codereview.chromium.org/549953003/diff/20001/chrome/browser/jumplist_...
File chrome/browser/jumplist_win.cc (right):

https://codereview.chromium.org/549953003/diff/20001/chrome/browser/jumplist_...
chrome/browser/jumplist_win.cc:402: PostRunUpdate();
Is it possible to not callout like this while holding a lock? It's easy to get
that wrong.

https://codereview.chromium.org/549953003/diff/20001/chrome/browser/jumplist_...
chrome/browser/jumplist_win.cc:440: base::AutoLock auto_lock(list_lock_);
Do you really need to hold the lock here?

https://codereview.chromium.org/549953003/diff/20001/chrome/browser/jumplist_...
File chrome/browser/jumplist_win.h (right):

https://codereview.chromium.org/549953003/diff/20001/chrome/browser/jumplist_...
chrome/browser/jumplist_win.h:46: // AddObserver() and register this class as an
observer, it automatically
Update comments and what not about new functionality. AddObserver is a poor name
too (as is AddObservers). SetProfile is a better name.

https://codereview.chromium.org/549953003/diff/20001/chrome/browser/jumplist_...
chrome/browser/jumplist_win.h:126: void OnIncognitoAvailabilityChanged();
Can this and the PostRunUpdate be private?

[Joao da Silva, 2014-09-08 16:44:11]

https://codereview.chromium.org/549953003/diff/20001/chrome/browser/jumplist_...
File chrome/browser/jumplist_win.cc (right):

https://codereview.chromium.org/549953003/diff/20001/chrome/browser/jumplist_...
chrome/browser/jumplist_win.cc:402: PostRunUpdate();
On 2014/09/08 16:15:17, sky wrote:
> Is it possible to not callout like this while holding a lock? It's easy to get
> that wrong.

Done.

https://codereview.chromium.org/549953003/diff/20001/chrome/browser/jumplist_...
chrome/browser/jumplist_win.cc:440: base::AutoLock auto_lock(list_lock_);
On 2014/09/08 16:15:17, sky wrote:
> Do you really need to hold the lock here?

The header documents that |icon_urls_| is protected by the lock. But there is no
need to hold it while calling PostRunUpdate, so fixed that.

https://codereview.chromium.org/549953003/diff/20001/chrome/browser/jumplist_...
File chrome/browser/jumplist_win.h (right):

https://codereview.chromium.org/549953003/diff/20001/chrome/browser/jumplist_...
chrome/browser/jumplist_win.h:46: // AddObserver() and register this class as an
observer, it automatically
On 2014/09/08 16:15:17, sky wrote:
> Update comments and what not about new functionality. AddObserver is a poor
name
> too (as is AddObservers). SetProfile is a better name.

Turns out that there is a single user of this class so it's quite easy to
sanitize the API and just pass the Profile in the ctor.

Removed AbbObserver and RemoveObserver.

https://codereview.chromium.org/549953003/diff/20001/chrome/browser/jumplist_...
chrome/browser/jumplist_win.h:126: void OnIncognitoAvailabilityChanged();
On 2014/09/08 16:15:17, sky wrote:
> Can this and the PostRunUpdate be private?

Done.

[sky, 2014-09-08 19:56:42]

LGTM

[Joao da Silva, 2014-09-08 21:13:00]

The CQ bit was checked by joaodasilva@chromium.org

[commit-bot: I haz the power, 2014-09-08 21:14:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/549953003/40001

[commit-bot: I haz the power, 2014-09-09 04:53:50]

Committed patchset #3 (id:40001) as 3bc6d03a02f60d901943f4422bfd6c0267c76059

[commit-bot: I haz the power, 2014-09-10 03:50:24]

Patchset 3 (id:??) landed as
https://crrev.com/b943d1c8690d35fc9aeaedaca39c32baddffc082
Cr-Commit-Position: refs/heads/master@{#293850}