[DevTools] Create last agents on attach, modify InspectorDatabaseAgent to support it.

[DevTools] Create last agents on attach, modify InspectorDatabaseAgent to support it.

We now ask DatabaseTracker about all open databases when creating the agent.

BUG=590878
TEST=http/tests/inspector-enabled/database-open.html

Committed: https://crrev.com/071a0f6f8ea7191da644265ec12df41216c68635
Cr-Commit-Position: refs/heads/master@{#387496}

[dgozman, 2016-04-12 21:01:40]

dgozman@chromium.org changed reviewers:
+ michaeln@chromium.org, pfeldman@chromium.org

[dgozman, 2016-04-12 21:01:41]

Could you please take a look?
michaeln@ - at modules/webdatabase.
pfeldman@ - at web.

Thanks,
Dmitry

[michaeln, 2016-04-13 22:18:18]

https://codereview.chromium.org/1880273002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webdatabase/DatabaseTracker.h (right):

https://codereview.chromium.org/1880273002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webdatabase/DatabaseTracker.h:68: void
registerAllDatabasesInInspector(InspectorDatabaseAgent*);
It'd be nice to not have a dependency on the inspector here.

We could do this more generically with a function that returns a set of all open
Databases. I guess it'd need to be a GarbageCollectedFinalized set of some kind
which traced each database in the set as a Member<>?

Or maybe a forEachOpenDatabase(...) that takes a callback of some kind.

https://codereview.chromium.org/1880273002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webdatabase/InspectorDatabaseAgent.cpp
(right):

https://codereview.chromium.org/1880273002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webdatabase/InspectorDatabaseAgent.cpp:225: if
(!context->isDocument())
you could ASSERT(isDocument()) since websql is no longer supported from worker
threads

https://codereview.chromium.org/1880273002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webdatabase/InspectorDatabaseAgent.cpp:227: if
(toDocument(context)->frame()->page() != m_page)
oh, you filter for this page only

https://codereview.chromium.org/1880273002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webdatabase/InspectorDatabaseAgent.cpp:257:
DatabaseTracker::tracker().registerAllDatabasesInInspector(this);
From the looks of the didCommitLoadForLocalFrame() method, it seems like the
agent class is scoped to a page. Should this inspector only know the set of
databases open in its page?

[dgozman, 2016-04-13 22:26:04]

https://codereview.chromium.org/1880273002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webdatabase/DatabaseTracker.h (right):

https://codereview.chromium.org/1880273002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webdatabase/DatabaseTracker.h:68: void
registerAllDatabasesInInspector(InspectorDatabaseAgent*);
On 2016/04/13 22:18:18, michaeln wrote:
> It'd be nice to not have a dependency on the inspector here.
> 
> We could do this more generically with a function that returns a set of all
open
> Databases. I guess it'd need to be a GarbageCollectedFinalized set of some
kind
> which traced each database in the set as a Member<>?
> 
> Or maybe a forEachOpenDatabase(...) that takes a callback of some kind.

I'd rather go for callback than Memeber<> collection to avoid all extra wrapping
and ownerhsip thoughts. I can't judge whether this particular class should not
depend on inspector, since there are a bunch of quite similarly named classes in
the same directory - it's hard to choose correct dependencies.

On the side note, I was surprised that DatabaseManager does not have a registry
of all databases and I had to look into DatabaseTracker.

https://codereview.chromium.org/1880273002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webdatabase/InspectorDatabaseAgent.cpp
(right):

https://codereview.chromium.org/1880273002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webdatabase/InspectorDatabaseAgent.cpp:257:
DatabaseTracker::tracker().registerAllDatabasesInInspector(this);
On 2016/04/13 22:18:18, michaeln wrote:
> From the looks of the didCommitLoadForLocalFrame() method, it seems like the
> agent class is scoped to a page. Should this inspector only know the set of
> databases open in its page?

That's why I've added filtering to didOpenDatabase. Do you think we'd better
pass Page* to registerAllDatabasesInInspector (or getAllDatabasesForPage)? It
does not really matter for me - some place has to filter them out.

[dgozman, 2016-04-14 17:31:13]

Description was changed from

==========
[DevTools] Create last agents on attach, modify InspectorDatabaseAgent to
support it.

We now ask DatabaseTracker about all open databases when creating the agent.

BUG=590878
==========

to

==========
[DevTools] Create last agents on attach, modify InspectorDatabaseAgent to
support it.

We now ask DatabaseTracker about all open databases when creating the agent.

BUG=590878
TEST=http/tests/inspector-enabled/database-open.html
==========

[dgozman, 2016-04-14 17:41:45]

All comments addressed, please take another look.

[michaeln, 2016-04-14 19:42:19]

thnx, modules/webdatabase lgtm

[pfeldman, 2016-04-14 20:27:49]

lgtm

[dgozman, 2016-04-14 20:36:00]

The CQ bit was checked by dgozman@chromium.org

[commit-bot: I haz the power, 2016-04-14 20:36:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1880273002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1880273002/40001

[commit-bot: I haz the power, 2016-04-14 21:46:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-14 21:46:45]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dgozman, 2016-04-14 23:25:56]

The CQ bit was checked by dgozman@chromium.org

[commit-bot: I haz the power, 2016-04-14 23:26:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1880273002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1880273002/40001

[commit-bot: I haz the power, 2016-04-15 00:47:57]

Description was changed from

==========
[DevTools] Create last agents on attach, modify InspectorDatabaseAgent to
support it.

We now ask DatabaseTracker about all open databases when creating the agent.

BUG=590878
TEST=http/tests/inspector-enabled/database-open.html
==========

to

==========
[DevTools] Create last agents on attach, modify InspectorDatabaseAgent to
support it.

We now ask DatabaseTracker about all open databases when creating the agent.

BUG=590878
TEST=http/tests/inspector-enabled/database-open.html
==========

[commit-bot: I haz the power, 2016-04-15 00:47:58]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-15 00:49:38]

Description was changed from

==========
[DevTools] Create last agents on attach, modify InspectorDatabaseAgent to
support it.

We now ask DatabaseTracker about all open databases when creating the agent.

BUG=590878
TEST=http/tests/inspector-enabled/database-open.html
==========

to

==========
[DevTools] Create last agents on attach, modify InspectorDatabaseAgent to
support it.

We now ask DatabaseTracker about all open databases when creating the agent.

BUG=590878
TEST=http/tests/inspector-enabled/database-open.html

Committed: https://crrev.com/071a0f6f8ea7191da644265ec12df41216c68635
Cr-Commit-Position: refs/heads/master@{#387496}
==========

[commit-bot: I haz the power, 2016-04-15 00:49:39]

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