Display third party cookies and site data counts in the WebsiteSettings UI.

Display third party cookies and site data counts in the Website Settings UI.


BUG=113688
TEST=BrowsingDataCookieHelperTest*

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=137109

[markusheintz_, 2012-04-27 09:55:20]

Please review my CL. Thanks a lot.

[markusheintz_, 2012-04-27 09:55:51]

Please review my CL.

[bauerb at google, 2012-04-27 16:12:50]

https://chromiumcodereview.appspot.com/10092013/diff/17001/chrome/browser/bro...
File chrome/browser/browsing_data_cookie_helper.cc (right):

https://chromiumcodereview.appspot.com/10092013/diff/17001/chrome/browser/bro...
chrome/browser/browsing_data_cookie_helper.cc:140:
scoped_ptr<net::CookieMonster::CanonicalCookie> cookie;
You can immediately initialize |cookie| with the newly created CanonicalCookie.

https://chromiumcodereview.appspot.com/10092013/diff/17001/chrome/browser/bro...
File chrome/browser/browsing_data_cookie_helper_unittest.cc (right):

https://chromiumcodereview.appspot.com/10092013/diff/17001/chrome/browser/bro...
chrome/browser/browsing_data_cookie_helper_unittest.cc:115:
ASSERT_TRUE(BrowserThread::CurrentlyOn(BrowserThread::UI));
This line does not test what you think it tests ;-) The problem is that IO
thread and UI thread are mapped to the same message loop here, so even if you
post this task to the IO thread, the check will succeed.

https://chromiumcodereview.appspot.com/10092013/diff/17001/chrome/browser/bro...
File chrome/browser/browsing_data_indexed_db_helper.h (right):

https://chromiumcodereview.appspot.com/10092013/diff/17001/chrome/browser/bro...
chrome/browser/browsing_data_indexed_db_helper.h:122: // This only mutates on
the UI thread.
Do we know that?

https://chromiumcodereview.appspot.com/10092013/diff/17001/chrome/browser/bro...
File chrome/browser/browsing_data_local_storage_helper.h (right):

https://chromiumcodereview.appspot.com/10092013/diff/17001/chrome/browser/bro...
chrome/browser/browsing_data_local_storage_helper.h:127: const std::set<GURL>&
pending_local_storage_info() const {
Including this one, we now have five methods in this class which just delegate
to a set, and they have four different naming schemes (if we consider AddFoo()
and GetFooCount() to follow the same scheme). That makes me a sad panda :(

https://chromiumcodereview.appspot.com/10092013/diff/17001/chrome/browser/con...
File chrome/browser/content_settings/local_shared_objects_container.cc (right):

https://chromiumcodereview.appspot.com/10092013/diff/17001/chrome/browser/con...
chrome/browser/content_settings/local_shared_objects_container.cc:61: // Count
cookies for the domain of the given |origin|.
It's actually pretty subtle what "cookies for the domain" means here, because it
doesn't mean "all cookies which match the origin", it means "all cookies which
cannot have been set by an origin which would be considered a third party".
Maybe you could add a comment clarifying that (in some way which is actually
understandable).

https://chromiumcodereview.appspot.com/10092013/diff/17001/chrome/browser/con...
chrome/browser/content_settings/local_shared_objects_container.cc:103: const
std::set<GURL> urls = session_storages()->pending_local_storage_info();
Is it on purpose that we only look at pending local storages here, but
GetLocalStorageCount() returns all local storages?

https://chromiumcodereview.appspot.com/10092013/diff/17001/chrome/browser/con...
File chrome/browser/content_settings/local_shared_objects_container.h (right):

https://chromiumcodereview.appspot.com/10092013/diff/17001/chrome/browser/con...
chrome/browser/content_settings/local_shared_objects_container.h:30: size_t
Size() const;
To keep the interface consistent, could you name this method GetObjectCount()?

https://chromiumcodereview.appspot.com/10092013/diff/17001/chrome/browser/web...
File chrome/browser/website_settings.cc (right):

https://chromiumcodereview.appspot.com/10092013/diff/17001/chrome/browser/web...
chrome/browser/website_settings.cc:412:
net::RegistryControlledDomainService::GetDomainAndRegistry(site_url_);
Doesn't SameDomainOrHost() do this internally?

https://chromiumcodereview.appspot.com/10092013/diff/17001/chrome/browser/web...
chrome/browser/website_settings.cc:422: // the exact wording.
Usually, we add strings to the GRD file even if they haven't been finalized yet.

[markusheintz_, 2012-05-10 16:32:36]

Alright let's continue this one.

https://chromiumcodereview.appspot.com/10092013/diff/17001/chrome/browser/bro...
File chrome/browser/browsing_data_cookie_helper.cc (right):

https://chromiumcodereview.appspot.com/10092013/diff/17001/chrome/browser/bro...
chrome/browser/browsing_data_cookie_helper.cc:140:
scoped_ptr<net::CookieMonster::CanonicalCookie> cookie;
On 2012/04/27 16:12:50, bauerb at google wrote:
> You can immediately initialize |cookie| with the newly created
CanonicalCookie.

Done.

https://chromiumcodereview.appspot.com/10092013/diff/17001/chrome/browser/bro...
File chrome/browser/browsing_data_cookie_helper_unittest.cc (right):

https://chromiumcodereview.appspot.com/10092013/diff/17001/chrome/browser/bro...
chrome/browser/browsing_data_cookie_helper_unittest.cc:115:
ASSERT_TRUE(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2012/04/27 16:12:50, bauerb at google wrote:
> This line does not test what you think it tests ;-) The problem is that IO
> thread and UI thread are mapped to the same message loop here, so even if you
> post this task to the IO thread, the check will succeed.

:( . Stupid me. I removed the line. I also removed the legacy DCHECKS that test
whether a method is run n Thread A or B.

https://chromiumcodereview.appspot.com/10092013/diff/17001/chrome/browser/bro...
File chrome/browser/browsing_data_indexed_db_helper.h (right):

https://chromiumcodereview.appspot.com/10092013/diff/17001/chrome/browser/bro...
chrome/browser/browsing_data_indexed_db_helper.h:122: // This only mutates on
the UI thread.
On 2012/04/27 16:12:50, bauerb at google wrote:
> Do we know that?

yes. However it is read on the Webkit thread

https://chromiumcodereview.appspot.com/10092013/diff/17001/chrome/browser/bro...
File chrome/browser/browsing_data_local_storage_helper.h (right):

https://chromiumcodereview.appspot.com/10092013/diff/17001/chrome/browser/bro...
chrome/browser/browsing_data_local_storage_helper.h:127: const std::set<GURL>&
pending_local_storage_info() const {
On 2012/04/27 16:12:50, bauerb at google wrote:
> Including this one, we now have five methods in this class which just delegate
> to a set, and they have four different naming schemes (if we consider AddFoo()
> and GetFooCount() to follow the same scheme). That makes me a sad panda :(

The style guide recommend to user Unix hacker style for accessors:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Names

But I really like to make you a happy Panda. So I converted this to the
GetLocalStorageInfo... naming scheme. The fact the pending_local_storage is
returned is an implementation detail. What the caller wants here is the current
list of local storage info objects. Therfor I name it GetLocalStorageInfo.

I really need to refactor the browsing data code. And I will do so soon :).

Hope that makes you a slightly less sad Panda. I'm also not yet happy.

https://chromiumcodereview.appspot.com/10092013/diff/17001/chrome/browser/con...
File chrome/browser/content_settings/local_shared_objects_container.cc (right):

https://chromiumcodereview.appspot.com/10092013/diff/17001/chrome/browser/con...
chrome/browser/content_settings/local_shared_objects_container.cc:61: // Count
cookies for the domain of the given |origin|.
On 2012/04/27 16:12:50, bauerb at google wrote:
> It's actually pretty subtle what "cookies for the domain" means here, because
it
> doesn't mean "all cookies which match the origin", it means "all cookies which
> cannot have been set by an origin which would be considered a third party".
> Maybe you could add a comment clarifying that (in some way which is actually
> understandable).

I hope the comment explains this better now.

https://chromiumcodereview.appspot.com/10092013/diff/17001/chrome/browser/con...
chrome/browser/content_settings/local_shared_objects_container.cc:103: const
std::set<GURL> urls = session_storages()->pending_local_storage_info();
On 2012/04/27 16:12:50, bauerb at google wrote:
> Is it on purpose that we only look at pending local storages here, but
> GetLocalStorageCount() returns all local storages?

Fixed.

https://chromiumcodereview.appspot.com/10092013/diff/17001/chrome/browser/con...
File chrome/browser/content_settings/local_shared_objects_container.h (right):

https://chromiumcodereview.appspot.com/10092013/diff/17001/chrome/browser/con...
chrome/browser/content_settings/local_shared_objects_container.h:30: size_t
Size() const;
On 2012/04/27 16:12:50, bauerb at google wrote:
> To keep the interface consistent, could you name this method GetObjectCount()?

Done.

https://chromiumcodereview.appspot.com/10092013/diff/17001/chrome/browser/web...
File chrome/browser/website_settings.cc (right):

https://chromiumcodereview.appspot.com/10092013/diff/17001/chrome/browser/web...
chrome/browser/website_settings.cc:412:
net::RegistryControlledDomainService::GetDomainAndRegistry(site_url_);
On 2012/04/27 16:12:50, bauerb at google wrote:
> Doesn't SameDomainOrHost() do this internally?

Yes it does.

Here I need to get the domain of the URL in order to display it in the UI.

https://chromiumcodereview.appspot.com/10092013/diff/17001/chrome/browser/web...
chrome/browser/website_settings.cc:422: // the exact wording.
On 2012/04/27 16:12:50, bauerb at google wrote:
> Usually, we add strings to the GRD file even if they haven't been finalized
yet.

Done.

[Bernhard Bauer, 2012-05-10 17:18:59]

Unless you plan to actually call the feature WebsiteSettOMGs (and I'm not
entirely adverse to that), you probably should rename the CL title ;-)

https://chromiumcodereview.appspot.com/10092013/diff/38005/chrome/browser/bro...
File chrome/browser/browsing_data_appcache_helper.h (right):

https://chromiumcodereview.appspot.com/10092013/diff/38005/chrome/browser/bro...
chrome/browser/browsing_data_appcache_helper.h:80: // Returns a current map with
the |AppChacheInfoVector|s per origin.
Nit: "AppCacheInfoVector"

https://chromiumcodereview.appspot.com/10092013/diff/38005/chrome/browser/bro...
File chrome/browser/browsing_data_database_helper.cc (right):

https://chromiumcodereview.appspot.com/10092013/diff/38005/chrome/browser/bro...
chrome/browser/browsing_data_database_helper.cc:147: return origin <
other.origin;
Doesn't this allow only one database per origin? In the old version you check
for name equality as well.

https://chromiumcodereview.appspot.com/10092013/diff/38005/chrome/browser/bro...
File chrome/browser/browsing_data_indexed_db_helper.cc (right):

https://chromiumcodereview.appspot.com/10092013/diff/38005/chrome/browser/bro...
chrome/browser/browsing_data_indexed_db_helper.cc:238: bool duplicate = false;
I think you don't need this anymore if you have a set?

https://chromiumcodereview.appspot.com/10092013/diff/38005/chrome/browser/bro...
File chrome/browser/browsing_data_local_storage_helper.h (right):

https://chromiumcodereview.appspot.com/10092013/diff/38005/chrome/browser/bro...
chrome/browser/browsing_data_local_storage_helper.h:88: // This only mutates in
the FILE thread.
That's not really true. 

These BrowsingDataHelper classes use this weird mixture of protecting access via
locks, via apartment threading, and via guard variables (which are themselves
protected via apartment threading). I think before we land this CL, we're going
to need to take a good look at thread safety for all of them.

[markusheintz_, 2012-05-11 12:18:28]

https://chromiumcodereview.appspot.com/10092013/diff/38005/chrome/browser/bro...
File chrome/browser/browsing_data_appcache_helper.h (right):

https://chromiumcodereview.appspot.com/10092013/diff/38005/chrome/browser/bro...
chrome/browser/browsing_data_appcache_helper.h:80: // Returns a current map with
the |AppChacheInfoVector|s per origin.
On 2012/05/10 17:18:59, Bernhard Bauer wrote:
> Nit: "AppCacheInfoVector"

Done.

https://chromiumcodereview.appspot.com/10092013/diff/38005/chrome/browser/bro...
File chrome/browser/browsing_data_database_helper.cc (right):

https://chromiumcodereview.appspot.com/10092013/diff/38005/chrome/browser/bro...
chrome/browser/browsing_data_database_helper.cc:147: return origin <
other.origin;
On 2012/05/10 17:18:59, Bernhard Bauer wrote:
> Doesn't this allow only one database per origin? In the old version you check
> for name equality as well.

Done. I think we can ignore the description here.

https://chromiumcodereview.appspot.com/10092013/diff/38005/chrome/browser/bro...
File chrome/browser/browsing_data_indexed_db_helper.cc (right):

https://chromiumcodereview.appspot.com/10092013/diff/38005/chrome/browser/bro...
chrome/browser/browsing_data_indexed_db_helper.cc:238: bool duplicate = false;
On 2012/05/10 17:18:59, Bernhard Bauer wrote:
> I think you don't need this anymore if you have a set?

Done.

https://chromiumcodereview.appspot.com/10092013/diff/38005/chrome/browser/bro...
File chrome/browser/browsing_data_local_storage_helper.h (right):

https://chromiumcodereview.appspot.com/10092013/diff/38005/chrome/browser/bro...
chrome/browser/browsing_data_local_storage_helper.h:88: // This only mutates in
the FILE thread.
On 2012/05/10 17:18:59, Bernhard Bauer wrote:
> That's not really true. 
> 
> These BrowsingDataHelper classes use this weird mixture of protecting access
via
> locks, via apartment threading, and via guard variables (which are themselves
> protected via apartment threading). I think before we land this CL, we're
going
> to need to take a good look at thread safety for all of them.

Yeah I forgot to mention the UI thread.

I checked it carefully but this code is *ughh* in a complicated state currently.

This comment was wrong. |BrowsingDataLocalStorageHelper| postes a task to the
FILE thread (see chrome/browser/browsing_data_local_storage_helper.h:79).
FetchLocalStorageInfo fetchs the LocalStorageInfo objects and adds them to the
|local_storage_info_| on the FILE thread.

The |CannedBrowsingDataLocalStorageHelper| posts a new task on the UI thread to
convert the PendingLocalStorageInfo objects to LocalStorageInfo objects and adds
them to the |local_storage_info_|.

So local_storage_info_ may mutate on the UI and FILE thread, but never during
the same hm.. "object life cycle".

[Bernhard Bauer, 2012-05-11 13:05:00]

https://chromiumcodereview.appspot.com/10092013/diff/38005/chrome/browser/bro...
File chrome/browser/browsing_data_local_storage_helper.h (right):

https://chromiumcodereview.appspot.com/10092013/diff/38005/chrome/browser/bro...
chrome/browser/browsing_data_local_storage_helper.h:88: // This only mutates in
the FILE thread.
On 2012/05/11 12:18:28, markusheintz_ wrote:
> On 2012/05/10 17:18:59, Bernhard Bauer wrote:
> > That's not really true. 
> > 
> > These BrowsingDataHelper classes use this weird mixture of protecting access
> via
> > locks, via apartment threading, and via guard variables (which are
themselves
> > protected via apartment threading). I think before we land this CL, we're
> going
> > to need to take a good look at thread safety for all of them.
> 
> Yeah I forgot to mention the UI thread.
> 
> I checked it carefully but this code is *ughh* in a complicated state
currently.
> 
> This comment was wrong. |BrowsingDataLocalStorageHelper| postes a task to the
> FILE thread (see chrome/browser/browsing_data_local_storage_helper.h:79).
> FetchLocalStorageInfo fetchs the LocalStorageInfo objects and adds them to the
> |local_storage_info_| on the FILE thread.
> 
> The |CannedBrowsingDataLocalStorageHelper| posts a new task on the UI thread
to
> convert the PendingLocalStorageInfo objects to LocalStorageInfo objects and
adds
> them to the |local_storage_info_|.
> 
> So local_storage_info_ may mutate on the UI and FILE thread, but never during
> the same hm.. "object life cycle".

Yeah, that's what I meant. Invariants of the form "only mutates on the X thread"
don't really help in terms of thread safety, because we still need to guard
against read/write race conditions. Invariants of the form "only mutates on the
X or Y thread" help even less, as this on its own actually means that it could
be concurrently modified :-D

The "object life cycle" thing is what I meant by guard variables: in this case,
we only access |local_storage_info_| while |is_fetching_| is true, and
|is_fetching_| is only *accessed* on the UI thread. So, I think the code is
correct here (as long as it's used correctly, i.e. as long as no one calls
StartFetching while we are already fetching), but we should update the comments
to make it easier to verify the thread safety.

https://chromiumcodereview.appspot.com/10092013/diff/39027/chrome/browser/bro...
File chrome/browser/browsing_data_indexed_db_helper.cc (right):

https://chromiumcodereview.appspot.com/10092013/diff/39027/chrome/browser/bro...
chrome/browser/browsing_data_indexed_db_helper.cc:189: const GURL& origin, const
string16& name) {
This doesn't match the declared signature anymore.

[markusheintz_, 2012-05-14 11:00:54]

https://chromiumcodereview.appspot.com/10092013/diff/38005/chrome/browser/bro...
File chrome/browser/browsing_data_local_storage_helper.h (right):

https://chromiumcodereview.appspot.com/10092013/diff/38005/chrome/browser/bro...
chrome/browser/browsing_data_local_storage_helper.h:88: // This only mutates in
the FILE thread.
On 2012/05/11 13:05:00, Bernhard Bauer wrote:
> On 2012/05/11 12:18:28, markusheintz_ wrote:
> > On 2012/05/10 17:18:59, Bernhard Bauer wrote:
> > > That's not really true. 
> > > 
> > > These BrowsingDataHelper classes use this weird mixture of protecting
access
> > via
> > > locks, via apartment threading, and via guard variables (which are
> themselves
> > > protected via apartment threading). I think before we land this CL, we're
> > going
> > > to need to take a good look at thread safety for all of them.
> > 
> > Yeah I forgot to mention the UI thread.
> > 
> > I checked it carefully but this code is *ughh* in a complicated state
> currently.
> > 
> > This comment was wrong. |BrowsingDataLocalStorageHelper| postes a task to
the
> > FILE thread (see chrome/browser/browsing_data_local_storage_helper.h:79).
> > FetchLocalStorageInfo fetchs the LocalStorageInfo objects and adds them to
the
> > |local_storage_info_| on the FILE thread.
> > 
> > The |CannedBrowsingDataLocalStorageHelper| posts a new task on the UI thread
> to
> > convert the PendingLocalStorageInfo objects to LocalStorageInfo objects and
> adds
> > them to the |local_storage_info_|.
> > 
> > So local_storage_info_ may mutate on the UI and FILE thread, but never
during
> > the same hm.. "object life cycle".
> 
> Yeah, that's what I meant. Invariants of the form "only mutates on the X
thread"
> don't really help in terms of thread safety, because we still need to guard
> against read/write race conditions. Invariants of the form "only mutates on
the
> X or Y thread" help even less, as this on its own actually means that it could
> be concurrently modified :-D
> 
> The "object life cycle" thing is what I meant by guard variables: in this
case,
> we only access |local_storage_info_| while |is_fetching_| is true, and
> |is_fetching_| is only *accessed* on the UI thread. So, I think the code is
> correct here (as long as it's used correctly, i.e. as long as no one calls
> StartFetching while we are already fetching), but we should update the
comments
> to make it easier to verify the thread safety.

I tried to make this comment a bit more useful.

https://chromiumcodereview.appspot.com/10092013/diff/39027/chrome/browser/bro...
File chrome/browser/browsing_data_indexed_db_helper.cc (right):

https://chromiumcodereview.appspot.com/10092013/diff/39027/chrome/browser/bro...
chrome/browser/browsing_data_indexed_db_helper.cc:189: const GURL& origin, const
string16& name) {
On 2012/05/11 13:05:00, Bernhard Bauer wrote:
> This doesn't match the declared signature anymore.

Done.

[Bernhard Bauer, 2012-05-14 13:29:04]

https://chromiumcodereview.appspot.com/10092013/diff/38012/chrome/browser/bro...
File chrome/browser/browsing_data_local_storage_helper.h (right):

https://chromiumcodereview.appspot.com/10092013/diff/38012/chrome/browser/bro...
chrome/browser/browsing_data_local_storage_helper.h:91: // accessed on the UI
thread.
Nice :)

[markusheintz_, 2012-05-14 15:06:45]

I updated the comments in the other browsing data helper (if applicable)

https://chromiumcodereview.appspot.com/10092013/diff/38012/chrome/browser/bro...
File chrome/browser/browsing_data_local_storage_helper.h (right):

https://chromiumcodereview.appspot.com/10092013/diff/38012/chrome/browser/bro...
chrome/browser/browsing_data_local_storage_helper.h:91: // accessed on the UI
thread.
On 2012/05/14 13:29:04, Bernhard Bauer wrote:
> Nice :)

Thanks

[markusheintz_, 2012-05-14 16:25:16]

Adding Glen to review the string change:
chrome/app/generated_resources.grd

[Glen Murphy, 2012-05-14 16:49:16]

lgtm

[Bernhard Bauer, 2012-05-14 17:14:06]

LGTM. There's still more cleanup to do here, but we can do that in future CLs
;-)

https://chromiumcodereview.appspot.com/10092013/diff/44002/chrome/browser/bro...
File chrome/browser/browsing_data_cookie_helper.h (right):

https://chromiumcodereview.appspot.com/10092013/diff/44002/chrome/browser/bro...
chrome/browser/browsing_data_cookie_helper.h:139: const GURL& frame_url,
Nit: indent

https://chromiumcodereview.appspot.com/10092013/diff/44002/chrome/browser/bro...
File chrome/browser/browsing_data_database_helper.cc (right):

https://chromiumcodereview.appspot.com/10092013/diff/44002/chrome/browser/bro...
chrome/browser/browsing_data_database_helper.cc:209:
BrowserThread::WEBKIT_DEPRECATED, FROM_HERE,
Have you found out in the mean time if there's a particular reason for doing
this on the WEBKIT thread?

https://chromiumcodereview.appspot.com/10092013/diff/44002/chrome/browser/bro...
File chrome/browser/browsing_data_database_helper.h (right):

https://chromiumcodereview.appspot.com/10092013/diff/44002/chrome/browser/bro...
chrome/browser/browsing_data_database_helper.h:160: // mutate on the UI or the
WEBKIT thread.
Nit: "it may be accessed"

https://chromiumcodereview.appspot.com/10092013/diff/44002/chrome/browser/bro...
File chrome/browser/browsing_data_indexed_db_helper.h (right):

https://chromiumcodereview.appspot.com/10092013/diff/44002/chrome/browser/bro...
chrome/browser/browsing_data_indexed_db_helper.h:123: // This only mutates on
the UI thread. But it is read on the Webkit thread.
If you're changing this comment, could you simplify it to something like
"protected by |lock_|"?

[markusheintz_, 2012-05-14 18:18:06]

Thanks for the review.

The plan to refactor all the canned browsing data helpers is almost ready. As
discussed offline I'll start working on this ASAP. It seems like this code (as I
took it over) got stuck in some refactoring phase. I'll clean this up.

I'm glad this change is in so that people can start testing the Website Settings
UI after the Win version is done. Especially from PM side there is a lot demand
for testing the UI.

https://chromiumcodereview.appspot.com/10092013/diff/44002/chrome/browser/bro...
File chrome/browser/browsing_data_cookie_helper.h (right):

https://chromiumcodereview.appspot.com/10092013/diff/44002/chrome/browser/bro...
chrome/browser/browsing_data_cookie_helper.h:139: const GURL& frame_url,
On 2012/05/14 17:14:07, Bernhard Bauer wrote:
> Nit: indent

Done.

https://chromiumcodereview.appspot.com/10092013/diff/44002/chrome/browser/bro...
File chrome/browser/browsing_data_database_helper.cc (right):

https://chromiumcodereview.appspot.com/10092013/diff/44002/chrome/browser/bro...
chrome/browser/browsing_data_database_helper.cc:209:
BrowserThread::WEBKIT_DEPRECATED, FROM_HERE,
On 2012/05/14 17:14:07, Bernhard Bauer wrote:
> Have you found out in the mean time if there's a particular reason for doing
> this on the WEBKIT thread?

I learned that most of the webkit code named Web* checks whether it is executed
on the Webkit thread or not. For some code there exists a workaround and after
initializing the code on the webkit thread it can be used on other threads as
well. I don't know all the details yet but I'm currently working on it. I was
also told there there is some work going on to fix these issues.

https://chromiumcodereview.appspot.com/10092013/diff/44002/chrome/browser/bro...
File chrome/browser/browsing_data_database_helper.h (right):

https://chromiumcodereview.appspot.com/10092013/diff/44002/chrome/browser/bro...
chrome/browser/browsing_data_database_helper.h:160: // mutate on the UI or the
WEBKIT thread.
On 2012/05/14 17:14:07, Bernhard Bauer wrote:
> Nit: "it may be accessed"

Done.

https://chromiumcodereview.appspot.com/10092013/diff/44002/chrome/browser/bro...
File chrome/browser/browsing_data_indexed_db_helper.h (right):

https://chromiumcodereview.appspot.com/10092013/diff/44002/chrome/browser/bro...
chrome/browser/browsing_data_indexed_db_helper.h:123: // This only mutates on
the UI thread. But it is read on the Webkit thread.
On 2012/05/14 17:14:07, Bernhard Bauer wrote:
> If you're changing this comment, could you simplify it to something like
> "protected by |lock_|"?

Done.

[commit-bot: I haz the power, 2012-05-14 18:21:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/markusheintz@chromium.org/10092013/38033

[commit-bot: I haz the power, 2012-05-14 19:03:22]

Try job failure for 10092013-38033 (retry) on mac_rel for step "unit_tests".
It's a second try, previously, step "unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-05-15 11:11:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/markusheintz@chromium.org/10092013/44007

[commit-bot: I haz the power, 2012-05-15 12:53:36]

Change committed as 137109