Implement download link in chrome://indexeddb-internals/

Implement download link in chrome://indexeddb-internals/

This closes all IDB connections for the given origin, zips up
the files and "downloads" them by initiating the download manager.
This must bounce events through a number of threads to manage temporary
files, callbacks into JavaScript, etc.

BUG=174188
TBR=jsbell@chromium.org

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

[alecflett, 2013-04-16 22:50:43]

So I'm happy with the implementation, but not happy with the new dependency.

I'm adding an explicit dependency from content/ to components/zip for
chrome://indexeddb-internals. I'm not sure what the best alternative is here.

The point of this (see the original bug too) is to allow ChromeOS users to dump
the IndexedDB part of their profiles and send them off for forensics. This may
actually have value elsewhere (I know chrome://net-internals allows you to
accumulate and send back logs)

jam@ - I'm particularly interested in your insight here.

[jam, 2013-04-16 23:00:24]

few comments:
-as a rule, src\content can't include src\components. i understand that this
particular component doesn't depend on src\content, but this is is a simple
layering rule that we should stick to (otherwise it becomes difficult to discern
what content can and can't include)
-since this is for debugging, does it need to be zipped? i.e. can you do some
other operation that combines the files without compression? do you really need
compression?

[alecflett, 2013-04-16 23:31:13]

On 2013/04/16 23:00:24, jam wrote:
> few comments:
> -as a rule, src\content can't include src\components. i understand that this
> particular component doesn't depend on src\content, but this is is a simple
> layering rule that we should stick to (otherwise it becomes difficult to
discern
> what content can and can't include)
> -since this is for debugging, does it need to be zipped? i.e. can you do some
> other operation that combines the files without compression? do you really
need
> compression?

I totally don't need compression but I do need containment - I need a single
file that can be e-mailed or uploaded somewhere. (The directory that is being
zipped up can potentially contain thousands of files)

dgrogan also suggested we move 'zip' somewhere else - it actually seems
strangely placed in components/zip because it is more or less self-contained and
doesn't have any browser-related features.

One final alternative is to put this in chrome/browser but I think there is
value in having this tool in content/ and the layer may still be ugly.

[dgrogan, 2013-04-16 23:32:31]

moving zip to base was briefly mentioned in
https://codereview.chromium.org/13257004

[jam, 2013-04-17 16:41:22]

On 2013/04/16 23:31:13, alecflett wrote:
> On 2013/04/16 23:00:24, jam wrote:
> > few comments:
> > -as a rule, src\content can't include src\components. i understand that this
> > particular component doesn't depend on src\content, but this is is a simple
> > layering rule that we should stick to (otherwise it becomes difficult to
> discern
> > what content can and can't include)
> > -since this is for debugging, does it need to be zipped? i.e. can you do
some
> > other operation that combines the files without compression? do you really
> need
> > compression?
> 
> I totally don't need compression but I do need containment - I need a single
> file that can be e-mailed or uploaded somewhere. (The directory that is being
> zipped up can potentially contain thousands of files)
> 
> dgrogan also suggested we move 'zip' somewhere else - it actually seems
> strangely placed in components/zip because it is more or less self-contained
and
> doesn't have any browser-related features.

I asked Brett why it was in components instead of base, and he said it's because
we don't want base to depend on zlib

> 
> One final alternative is to put this in chrome/browser but I think there is
> value in having this tool in content/ and the layer may still be ugly.

by "this" you mean the indexeddb webui? that should be in content, otherwise the
content api would need to expose internal stuff just for the debugging page.
that's the reason we have webui's in content.

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

https://codereview.chromium.org/13949013/diff/4001/content/public/browser/ind...
content/public/browser/indexed_db_context.h:32: virtual void ForceClose(const
GURL& origin_url) = 0;
there shouldn't be changes to the content api for methods that are called
internally by content. just leave these on the Impl object and feel free to
static_cast to it inside content (this is what's done with the webkit api as
well)

[alecflett, 2013-04-17 22:08:34]

On 2013/04/17 16:41:22, jam wrote:
> On 2013/04/16 23:31:13, alecflett wrote:
> > On 2013/04/16 23:00:24, jam wrote:
> > > few comments:
> > > -as a rule, src\content can't include src\components. i understand that
this
> > > particular component doesn't depend on src\content, but this is is a
simple
> > > layering rule that we should stick to (otherwise it becomes difficult to
> > discern
> > > what content can and can't include)
> > > -since this is for debugging, does it need to be zipped? i.e. can you do
> some
> > > other operation that combines the files without compression? do you really
> > need
> > > compression?
> > 
> > I totally don't need compression but I do need containment - I need a single
> > file that can be e-mailed or uploaded somewhere. (The directory that is
being
> > zipped up can potentially contain thousands of files)
> > 
> > dgrogan also suggested we move 'zip' somewhere else - it actually seems
> > strangely placed in components/zip because it is more or less self-contained
> and
> > doesn't have any browser-related features.
> 
> I asked Brett why it was in components instead of base, and he said it's
because
> we don't want base to depend on zlib
> 
> > 
> > One final alternative is to put this in chrome/browser but I think there is
> > value in having this tool in content/ and the layer may still be ugly.
> 
> by "this" you mean the indexeddb webui? that should be in content, otherwise
the
> content api would need to expose internal stuff just for the debugging page.
> that's the reason we have webui's in content.
> 
>
https://codereview.chromium.org/13949013/diff/4001/content/public/browser/ind...
> File content/public/browser/indexed_db_context.h (right):
> 
>
https://codereview.chromium.org/13949013/diff/4001/content/public/browser/ind...
> content/public/browser/indexed_db_context.h:32: virtual void ForceClose(const
> GURL& origin_url) = 0;
> there shouldn't be changes to the content api for methods that are called
> internally by content. just leave these on the Impl object and feel free to
> static_cast to it inside content (this is what's done with the webkit api as
> well)

Done. I'm glad to hear this :)

I'll leave the zip discussion to the mailing list, but now this patch can now be
limited in scope to just IDB stuff (i.e. without changing the content API) once
the zip stuff is settled. I've updated the patch appropriately.

dgrogan / jsbell - can I get a review for the idb stuff?

[jsbell, 2013-04-18 22:08:52]

Minor style nits. Logic seems fine.

https://codereview.chromium.org/13949013/diff/11009/content/browser/indexed_d...
File content/browser/indexed_db/indexed_db_internals_ui.cc (right):

https://codereview.chromium.org/13949013/diff/11009/content/browser/indexed_d...
content/browser/indexed_db/indexed_db_internals_ui.cc:229: LOG(ERROR) << "Origin
data ready in " << zip_path.value();
Don't forget to remove this.

https://codereview.chromium.org/13949013/diff/11009/content/browser/indexed_d...
content/browser/indexed_db/indexed_db_internals_ui.cc:259: LOG(ERROR) << "I'd
start to download: " << url.spec();
Don't forget to remove this.

https://codereview.chromium.org/13949013/diff/11009/content/browser/indexed_d...
content/browser/indexed_db/indexed_db_internals_ui.cc:287: } break;
Put break; inside braces?

https://codereview.chromium.org/13949013/diff/11009/content/browser/indexed_d...
content/browser/indexed_db/indexed_db_internals_ui.cc:307: << "Error downloading
database dump: " << net::ErrorToString(error);
Nit: Put first bit of log on previous line. (I think that's more standard)

[alecflett, 2013-04-29 17:22:59]

ok, this has been cleaned up to address review comments and use the new location
of zip/ from https://chromiumcodereview.appspot.com/14021015/ 

Note that these went through clang-format -style=Chromium, so I'd like to keep
the formatting/style that it uses there.

jsbell@ - review for indexed_db stuff?
jam@ - review for content/DEPS and content_browser.gypi?

[jsbell, 2013-04-29 19:16:13]

lgtm

https://codereview.chromium.org/13949013/diff/20001/content/browser/indexed_d...
File content/browser/indexed_db/indexed_db_context_impl.h (left):

https://codereview.chromium.org/13949013/diff/20001/content/browser/indexed_d...
content/browser/indexed_db/indexed_db_context_impl.h:115: bool
IsInOriginSet(const GURL& origin_url) {
Just a minor note about the name change here (IsInOriginSet -> HasOrigin). 

The previous naming of these 3 methods
(AddToOriginSet/RemoveOriginSet/IsInOriginSet) implied that a
IndexedDBContextImpl "has a" collection of origins. And, of course, there's
GetOriginSet which returns the actual set.

The name HasOrigin implies that IndexedDBContextImpl "is a" collection of
origins, but the other methods still have the old implication, and the set is
still directly accessible via GetOriginSet.

I'm not sure what I'm suggesting here, but the rename feels a bit arbitrary.

https://codereview.chromium.org/13949013/diff/20001/content/browser/indexed_d...
File content/browser/indexed_db/indexed_db_context_impl.h (right):

https://codereview.chromium.org/13949013/diff/20001/content/browser/indexed_d...
content/browser/indexed_db/indexed_db_context_impl.h:57: void
SetForceKeepSessionState() { force_keep_session_state_ = true; }
Looks like you ran clang-format over this?

[alecflett, 2013-04-29 22:28:15]

idb issues fixed.

https://codereview.chromium.org/13949013/diff/20001/content/browser/indexed_d...
File content/browser/indexed_db/indexed_db_context_impl.h (left):

https://codereview.chromium.org/13949013/diff/20001/content/browser/indexed_d...
content/browser/indexed_db/indexed_db_context_impl.h:115: bool
IsInOriginSet(const GURL& origin_url) {
On 2013/04/29 19:16:14, jsbell wrote:
> Just a minor note about the name change here (IsInOriginSet -> HasOrigin). 
> 
> The previous naming of these 3 methods
> (AddToOriginSet/RemoveOriginSet/IsInOriginSet) implied that a
> IndexedDBContextImpl "has a" collection of origins. And, of course, there's
> GetOriginSet which returns the actual set.
> 
> The name HasOrigin implies that IndexedDBContextImpl "is a" collection of
> origins, but the other methods still have the old implication, and the set is
> still directly accessible via GetOriginSet.
> 
> I'm not sure what I'm suggesting here, but the rename feels a bit arbitrary.

oops this is somewhat leftover from an earlier version of the patch where I
thought I was going to be exposing this as a virtual from IndexedDBContext* -
I'll put it back, but I will need to make it public

FWIW I find the verb-preposition form awkward ("AddToOriginSet" instead of
"AddOrigin") but I won't fix that here

https://codereview.chromium.org/13949013/diff/20001/content/browser/indexed_d...
File content/browser/indexed_db/indexed_db_context_impl.h (right):

https://codereview.chromium.org/13949013/diff/20001/content/browser/indexed_d...
content/browser/indexed_db/indexed_db_context_impl.h:57: void
SetForceKeepSessionState() { force_keep_session_state_ = true; }
On 2013/04/29 19:16:14, jsbell wrote:
> Looks like you ran clang-format over this?

yes.

[commit-bot: I haz the power, 2013-05-03 23:18:17]

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

[commit-bot: I haz the power, 2013-05-04 01:52:49]

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

[commit-bot: I haz the power, 2013-05-04 02:54:38]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
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-05-04 04:21:38]

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

[commit-bot: I haz the power, 2013-05-04 05:07:59]

Retried try job too often on ios_rel_device for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...

[commit-bot: I haz the power, 2013-05-04 05:13:49]

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

[commit-bot: I haz the power, 2013-05-04 05:24:45]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
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-05-04 05:24:49]

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

[commit-bot: I haz the power, 2013-05-04 06:05:25]

Retried try job too often on ios_dbg_simulator for step(s) base_unittests,
compile, content_unittests, crypto_unittests, googleurl_unittests,
media_unittests, net_unittests, sql_unittests, ui_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[commit-bot: I haz the power, 2013-05-04 06:09:19]

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

[commit-bot: I haz the power, 2013-05-04 06:40:22]

Retried try job too often on ios_rel_device for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...

[commit-bot: I haz the power, 2013-05-04 13:28:27]

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

[commit-bot: I haz the power, 2013-05-04 13:59:13]

Retried try job too often on ios_rel_device for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...

[commit-bot: I haz the power, 2013-05-05 05:03:20]

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

[commit-bot: I haz the power, 2013-05-05 05:16:03]

Retried try job too often on ios_dbg_simulator for step(s) base_unittests,
compile, content_unittests, crypto_unittests, googleurl_unittests,
media_unittests, net_unittests, sql_unittests, ui_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[commit-bot: I haz the power, 2013-05-06 12:47:51]

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

[commit-bot: I haz the power, 2013-05-06 13:03:21]

Retried try job too often on ios_rel_device for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...

[alecflett, 2013-05-06 16:26:29]

Committed patchset #14 manually as r198469 (presubmit successful).