Add SiteInstance parameters to IndexedDBContext calls

Add SiteInstance parameters to IndexedDBContext calls


BUG=

[alecflett, 2012-09-10 18:38:30]

I borrowed a lot of stuff from ericu's patch, and tried to generalize it a bit
so that we didn't have to keep getting the StoragePartition for each storage
system. 

ajwong, mind taking a look?

[awong, 2012-09-10 21:55:21]

Looks pretty solid. Thanks for doing this!

RE data deleter, I think it's better to keep it uniformly broken for now.

At the very least, I'd like to change data deleter in a different CL since I
don't fully understand how it works so the chance of a behavior change causing
us to want to revert the whole CL is higher.

http://codereview.chromium.org/10836216/diff/2001/chrome/browser/browsing_dat...
File chrome/browser/browsing_data/browsing_data_indexed_db_helper.cc (right):

http://codereview.chromium.org/10836216/diff/2001/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_indexed_db_helper.cc:16: #include
"content/public/browser/browser_context.h"
Why do you need browser_context.h?

http://codereview.chromium.org/10836216/diff/2001/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_indexed_db_helper.cc:31: explicit
BrowsingDataIndexedDBHelperImpl(IndexedDBContext* context);
Call this indexed_db_context.   Yeah it's long. :(

http://codereview.chromium.org/10836216/diff/2001/chrome/browser/browsing_dat...
File chrome/browser/browsing_data/browsing_data_indexed_db_helper.h (right):

http://codereview.chromium.org/10836216/diff/2001/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_indexed_db_helper.h:20: class
Profile;
Newline before the namespace opening line.

http://codereview.chromium.org/10836216/diff/2001/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_indexed_db_helper.h:22: class
SiteInstance;
SiteInstance doesn't needed here. Remove?

http://codereview.chromium.org/10836216/diff/2001/chrome/browser/extensions/d...
File chrome/browser/extensions/data_deleter.cc (right):

http://codereview.chromium.org/10836216/diff/2001/chrome/browser/extensions/d...
chrome/browser/extensions/data_deleter.cc:89: // isolated app, or regular app.
I think for right now, just use GetDefaultStoragePartition() for all cases
similar to how database_tracker_ works above.

That way, the various data types are are consistently broken. :)

http://codereview.chromium.org/10836216/diff/2001/chrome/browser/extensions/d...
chrome/browser/extensions/data_deleter.cc:90: content::StoragePartition*
storage_partition;
Initialize to NULL?

http://codereview.chromium.org/10836216/diff/2001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/cookies_view_handler.cc (right):

http://codereview.chromium.org/10836216/diff/2001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/cookies_view_handler.cc:194:
content::WebContents* contents = web_ui()->GetWebContents();
This isn't quite right.  The StoragePartition for web_ui() isn't always the same
as the default one.  The CookiesTreeModel doesn't understand how to display
multile partitions yet, so it's most correct to keep this with
GetDefaultStoragePartition(storage_partiton).

[alecflett, 2012-09-10 23:14:55]

All issues addressed...

http://codereview.chromium.org/10836216/diff/2001/chrome/browser/browsing_dat...
File chrome/browser/browsing_data/browsing_data_indexed_db_helper.cc (right):

http://codereview.chromium.org/10836216/diff/2001/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_indexed_db_helper.cc:16: #include
"content/public/browser/browser_context.h"
On 2012/09/10 21:55:21, awong wrote:
> Why do you need browser_context.h?

oops, don't now.

http://codereview.chromium.org/10836216/diff/2001/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_indexed_db_helper.cc:31: explicit
BrowsingDataIndexedDBHelperImpl(IndexedDBContext* context);
On 2012/09/10 21:55:21, awong wrote:
> Call this indexed_db_context.   Yeah it's long. :(

Done.

http://codereview.chromium.org/10836216/diff/2001/chrome/browser/browsing_dat...
File chrome/browser/browsing_data/browsing_data_indexed_db_helper.h (right):

http://codereview.chromium.org/10836216/diff/2001/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_indexed_db_helper.h:20: class
Profile;
On 2012/09/10 21:55:21, awong wrote:
> Newline before the namespace opening line.

Done.

http://codereview.chromium.org/10836216/diff/2001/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_indexed_db_helper.h:22: class
SiteInstance;
On 2012/09/10 21:55:21, awong wrote:
> SiteInstance doesn't needed here. Remove?

Done.

http://codereview.chromium.org/10836216/diff/2001/chrome/browser/extensions/d...
File chrome/browser/extensions/data_deleter.cc (right):

http://codereview.chromium.org/10836216/diff/2001/chrome/browser/extensions/d...
chrome/browser/extensions/data_deleter.cc:89: // isolated app, or regular app.
On 2012/09/10 21:55:21, awong wrote:
> I think for right now, just use GetDefaultStoragePartition() for all cases
> similar to how database_tracker_ works above.
> 
> That way, the various data types are are consistently broken. :)

Heh, that seems very sad. But ok...it vastly simplifies this stuff then.

http://codereview.chromium.org/10836216/diff/2001/chrome/browser/extensions/d...
chrome/browser/extensions/data_deleter.cc:90: content::StoragePartition*
storage_partition;
On 2012/09/10 21:55:21, awong wrote:
> Initialize to NULL?

I specifically didn't so clang would complain if I didn't.

(because currently there's no execution path that doesn't set storage_partition)

but in any case this becomes moot when I follow the above comment.

[awong, 2012-09-10 23:17:52]

LGTM

Thanks!

http://codereview.chromium.org/10836216/diff/2001/chrome/browser/extensions/d...
File chrome/browser/extensions/data_deleter.cc (right):

http://codereview.chromium.org/10836216/diff/2001/chrome/browser/extensions/d...
chrome/browser/extensions/data_deleter.cc:89: // isolated app, or regular app.
On 2012/09/10 23:14:55, alecflett wrote:
> On 2012/09/10 21:55:21, awong wrote:
> > I think for right now, just use GetDefaultStoragePartition() for all cases
> > similar to how database_tracker_ works above.
> > 
> > That way, the various data types are are consistently broken. :)
> 
> Heh, that seems very sad. But ok...it vastly simplifies this stuff then.

Yeah it's sad...there's an outstanding bug to fix this.  It's actually next on
the hit list if you felt the urge to pick it up. Otherwise, I'll be looking at
it very soon.

[commit-bot: I haz the power, 2012-09-10 23:23:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alecflett@chromium.org/10836216/5003

[commit-bot: I haz the power, 2012-09-10 23:23:57]

Presubmit check for 10836216-5003 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui/webui/options
    content/browser/renderer_host
    content/browser
    chrome/browser/extensions
    content/public/browser
    chrome/browser/browsing_data

Presubmit checks took 1.1s to calculate.

[awong, 2012-09-11 21:43:03]

FYI, I just committed
http://src.chromium.org/viewvc/chrome?view=rev&revision=156126 which will cause
a small merge on your side.

On 2012/09/10 23:23:57, I haz the power (commit-bot) wrote:
> Presubmit check for 10836216-5003 failed and returned exit status 1.
> 
> 
> Running presubmit commit checks ...
> 
> ** Presubmit Messages **
> If this change has an associated bug, add BUG=[bug number].
> 
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for files in these directories:
>     chrome/browser/ui/webui/options
>     content/browser/renderer_host
>     content/browser
>     chrome/browser/extensions
>     content/public/browser
>     chrome/browser/browsing_data
> 
> Presubmit checks took 1.1s to calculate.

[michaeln, 2012-09-12 00:39:57]

Also would be good to add BUG=85121 to the CL description.

[awong, 2012-09-13 18:32:53]

On 2012/09/12 00:39:57, michaeln wrote:
> Also would be good to add BUG=85121 to the CL description.

ping!  If you'd like, I'm happy to merge + land this for you.

[alecflett, 2012-09-13 20:43:18]

I got caught up in some other things. two open pieces:
1) I missed some tests. oops.

2) needs OWNERS approval across a few modules.

[awong, 2012-09-14 00:16:13]

On 2012/09/13 20:43:18, alecflett wrote:
> I got caught up in some other things. two open pieces:
> 1) I missed some tests. oops.
> 
> 2) needs OWNERS approval across a few modules.

No worries.  Will you be able to get this in before tomorrow afternoon?  I'd
love to have this in by the weekend so we don't miss the m23 branchcut.

[alecflett, 2012-09-14 19:24:24]

On 2012/09/14 00:16:13, awong wrote:
> On 2012/09/13 20:43:18, alecflett wrote:
> > I got caught up in some other things. two open pieces:
> > 1) I missed some tests. oops.
> > 
> > 2) needs OWNERS approval across a few modules.
> 
> No worries.  Will you be able to get this in before tomorrow afternoon?  I'd
> love to have this in by the weekend so we don't miss the m23 branchcut.

going to do everything I can to get this in today!

[awong, 2012-09-14 23:05:29]

On 2012/09/14 19:24:24, alecflett wrote:
> On 2012/09/14 00:16:13, awong wrote:
> > On 2012/09/13 20:43:18, alecflett wrote:
> > > I got caught up in some other things. two open pieces:
> > > 1) I missed some tests. oops.
> > > 
> > > 2) needs OWNERS approval across a few modules.
> > 
> > No worries.  Will you be able to get this in before tomorrow afternoon?  I'd
> > love to have this in by the weekend so we don't miss the m23 branchcut.
> 
> going to do everything I can to get this in today!

:D  Cool. Let me know know if anything comes up.  michaeln and I are
chattering/whacking at appcache and FSContext in parallel so we're really
plugged into this chunk of code right now.

[alecflett, 2012-09-14 23:16:46]

On 2012/09/14 23:05:29, awong wrote:
> On 2012/09/14 19:24:24, alecflett wrote:
> > On 2012/09/14 00:16:13, awong wrote:
> > > On 2012/09/13 20:43:18, alecflett wrote:
> > > > I got caught up in some other things. two open pieces:
> > > > 1) I missed some tests. oops.
> > > > 
> > > > 2) needs OWNERS approval across a few modules.
> > > 
> > > No worries.  Will you be able to get this in before tomorrow afternoon? 
I'd
> > > love to have this in by the weekend so we don't miss the m23 branchcut.
> > 
> > going to do everything I can to get this in today!
> 
> :D  Cool. Let me know know if anything comes up.  michaeln and I are
> chattering/whacking at appcache and FSContext in parallel so we're really
> plugged into this chunk of code right now.

Ugh, I ran out of time trying to finish something ELSE before my other reviewer
flies back to Japan tomorrow! I can try to get to this over the weekend, but not
likely to land until monday, feel free to take over.. sorry.

[awong, 2012-09-14 23:18:01]

On 2012/09/14 23:16:46, alecflett wrote:
> On 2012/09/14 23:05:29, awong wrote:
> > On 2012/09/14 19:24:24, alecflett wrote:
> > > On 2012/09/14 00:16:13, awong wrote:
> > > > On 2012/09/13 20:43:18, alecflett wrote:
> > > > > I got caught up in some other things. two open pieces:
> > > > > 1) I missed some tests. oops.
> > > > > 
> > > > > 2) needs OWNERS approval across a few modules.
> > > > 
> > > > No worries.  Will you be able to get this in before tomorrow afternoon? 
> I'd
> > > > love to have this in by the weekend so we don't miss the m23 branchcut.
> > > 
> > > going to do everything I can to get this in today!
> > 
> > :D  Cool. Let me know know if anything comes up.  michaeln and I are
> > chattering/whacking at appcache and FSContext in parallel so we're really
> > plugged into this chunk of code right now.
> 
> Ugh, I ran out of time trying to finish something ELSE before my other
reviewer
> flies back to Japan tomorrow! I can try to get to this over the weekend, but
not
> likely to land until monday, feel free to take over.. sorry.

No worries. I'll clone the CL and pick it from here. Thanks! And good luck
getting stuff done before your reviewer leaves.

/me is jealous...he wants to fly to Japan tomorrow too.