Refactor IndexedDBInfo out of browsingdata

Refactor IndexedDBInfo out of browsingdata

The struct used for the "site content" UI is also going to be
useful for chrome://indexeddb-internals, so I've factored that out
so it can be reused. This patch doesn't reuse it, but it will be
in a future patch.

BUG=174188
TBR=sail@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194063

[alecflett, 2013-04-09 23:06:39]

joi - review for new function in content/public/browser?

estade - review for gtk, and chrome/browser/ui/webui ?

michaeln - review for browsing_data stuff?

jsbell - general idb API stuff, since you wrote the original IndexedDBInfo ?

[michaeln, 2013-04-10 00:47:25]

lgtm

https://codereview.chromium.org/13954002/diff/3001/chrome/browser/browsing_da...
File chrome/browser/browsing_data/mock_browsing_data_indexed_db_helper.cc
(right):

https://codereview.chromium.org/13954002/diff/3001/chrome/browser/browsing_da...
chrome/browser/browsing_data/mock_browsing_data_indexed_db_helper.cc:31:
content::IndexedDBInfo info1 = { kOrigin1, 1, base::Time() };
<aside>
this looks fine, but i think is it generally more common for us to have ctors to
perform struct initialization than using c style syntax (not positive about
that, just an impression)? idk, if our style guide has any recommendation.
</aside>

https://codereview.chromium.org/13954002/diff/3001/content/public/browser/ind...
File content/public/browser/indexed_db_context.h (right):

https://codereview.chromium.org/13954002/diff/3001/content/public/browser/ind...
content/public/browser/indexed_db_context.h:11: #include
"base/files/file_path.h"
is this one needed?

[michaeln, 2013-04-10 00:51:17]

https://codereview.chromium.org/13954002/diff/1/chrome/browser/browsing_data/...
File chrome/browser/browsing_data/browsing_data_indexed_db_helper.cc (right):

https://codereview.chromium.org/13954002/diff/1/chrome/browser/browsing_data/...
chrome/browser/browsing_data/browsing_data_indexed_db_helper.cc:110: const
IndexedDBInfo& origin = *iter;
nit: maybe change this variable's name to 'info' for clarity

[Evan Stade, 2013-04-10 05:12:17]

lgtm, feel free to TBR OWNERS reviews of this sort.

[Jói, 2013-04-10 09:02:45]

LGTM for content/public

[alecflett, 2013-04-10 17:17:17]

https://codereview.chromium.org/13954002/diff/3001/chrome/browser/browsing_da...
File chrome/browser/browsing_data/mock_browsing_data_indexed_db_helper.cc
(right):

https://codereview.chromium.org/13954002/diff/3001/chrome/browser/browsing_da...
chrome/browser/browsing_data/mock_browsing_data_indexed_db_helper.cc:31:
content::IndexedDBInfo info1 = { kOrigin1, 1, base::Time() };
On 2013/04/10 00:47:26, michaeln wrote:
> <aside>
> this looks fine, but i think is it generally more common for us to have ctors
to
> perform struct initialization than using c style syntax (not positive about
> that, just an impression)? idk, if our style guide has any recommendation.
> </aside>

Yeah the style guide doesn't offer specific guidance here - part of the issue is
that these are structs used in an interface - in the spirit of keeping the
interface clean of implementations, I left out a constructor. I'll leave it in
but an alternative would have been a more wordy:

content::IndexedDBInfo info1;
info1.url = kOrigin1;
info1.size = 1;
info1.last_modified = base::Time();

which at least for the sake of the tests seemed like a step backwards.

https://codereview.chromium.org/13954002/diff/3001/content/public/browser/ind...
File content/public/browser/indexed_db_context.h (right):

https://codereview.chromium.org/13954002/diff/3001/content/public/browser/ind...
content/public/browser/indexed_db_context.h:11: #include
"base/files/file_path.h"
On 2013/04/10 00:47:26, michaeln wrote:
> is this one needed?

It is actually - it was implicitly included before this reshuffle.

[commit-bot: I haz the power, 2013-04-10 17:17:39]

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

[commit-bot: I haz the power, 2013-04-10 17:53:02]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-10 18:17:17]

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

[commit-bot: I haz the power, 2013-04-10 21:10:15]

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

[commit-bot: I haz the power, 2013-04-10 21:12:29]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[alecflett, 2013-04-10 21:45:29]

So I had to fix the mac cocoa build and I ended up switching to constructors
mostly because a subsequent patch (which adds new fields) triggered the
chromium-style warning that wants an out-of-line constructor... so, nice call
michaeln!

So I've added it.. but I'll now need jam's review for the new file in
content/public/browser with the constructor

[jam, 2013-04-11 02:18:54]

On 2013/04/10 21:45:29, alecflett wrote:
> So I had to fix the mac cocoa build and I ended up switching to constructors
> mostly because a subsequent patch (which adds new fields) triggered the
> chromium-style warning that wants an out-of-line constructor... so, nice call
> michaeln!
> 
> So I've added it.. but I'll now need jam's review for the new file in
> content/public/browser with the constructor

per content api convention, each class/struct/enum should be in a separate file,
so IndexedDBInfo needs to move out. if you can avoid the cc file without clang
warnings, that's a bit better (we try to avoid cc files in the content/public
directory unless clang forces us to).

[alecflett, 2013-04-11 17:53:17]

On 2013/04/11 02:18:54, jam wrote:

> per content api convention, each class/struct/enum should be in a separate
file,
> so IndexedDBInfo needs to move out. if you can avoid the cc file without clang
> warnings, that's a bit better (we try to avoid cc files in the content/public
> directory unless clang forces us to).

jam@ - Done. I wish I could have avoided the .cc but I'm adding additional
fields in later patches, and they trigger clang, as you mention..

[jam, 2013-04-11 18:19:25]

looks like you're missing the new header

https://codereview.chromium.org/13954002/diff/56001/content/public/browser/in...
File content/public/browser/indexed_db_context.h (right):

https://codereview.chromium.org/13954002/diff/56001/content/public/browser/in...
content/public/browser/indexed_db_context.h:11: #include
"base/files/file_path.h"
nit: don't need to add all these headers now

[alecflett, 2013-04-11 18:49:32]

On 2013/04/11 18:19:25, jam wrote:
> looks like you're missing the new header
> 
>
https://codereview.chromium.org/13954002/diff/56001/content/public/browser/in...
> File content/public/browser/indexed_db_context.h (right):
> 
>
https://codereview.chromium.org/13954002/diff/56001/content/public/browser/in...
> content/public/browser/indexed_db_context.h:11: #include
> "base/files/file_path.h"
> nit: don't need to add all these headers now

oops. header added, extra #includes removed.

[alecflett, 2013-04-11 22:17:14]

jam@ - this is ready to go - I ended up putting base/files/file_path.h in there
-  in the spirit of IWYU. It's included implicitly already. There's a method
here that uses base::FilePath, and in a later patch, I hit this while trying to
#include "indexed_db_context.h" without other #includes.

[jam, 2013-04-12 16:43:01]

lgtm

[commit-bot: I haz the power, 2013-04-12 17:23:48]

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

[commit-bot: I haz the power, 2013-04-12 17:54:32]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-12 17:58:52]

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

[commit-bot: I haz the power, 2013-04-12 18:13:11]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-12 21:18:34]

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

[commit-bot: I haz the power, 2013-04-12 22:32:52]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-12 22:32:59]

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

[commit-bot: I haz the power, 2013-04-13 01:25:02]

Change committed as 194063