Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2902673003:
Added a refresh database button on the IndexedDB view. (Closed)
|
Created:
3 years, 7 months ago by kristipark Modified:
3 years, 6 months ago CC:
chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, pfeldman, kozyatinskiy+blink_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdded a refresh database button on the IndexedDB view.
BUG=723871
Review-Url: https://codereview.chromium.org/2902673003
Cr-Commit-Position: refs/heads/master@{#475739}
Committed: https://chromium.googlesource.com/chromium/src/+/075f218d5fc98ed1738637f05354ce9a3b0cb831
Patch Set 1 #
Total comments: 5
Patch Set 2 : Removed model and test line #
Total comments: 8
Patch Set 3 : Changed to use refreshDatabase() function instead of event listener, created database structure in … #
Total comments: 15
Patch Set 4 : Finished test if refresh button refreshes the objectstore view. #Patch Set 5 : Removed a comment. #Patch Set 6 : Fixing trybot failures, wrong security origin in test expected file. #
Messages
Total messages: 38 (18 generated)
Description was changed from ========== Added a refresh database button on the IndexedDB view. BUG=723871 ========== to ========== Added a refresh database button on the IndexedDB view. BUG=723871 R=dmurph@chromium.org, eostroukhov@chromium.org ==========
kristipark@chromium.org changed reviewers: + dmurph@chromium.org, eostroukhov@chromium.org
Please let me know if my test file is on the right track.
eostroukhov@chromium.org changed reviewers: + caseq@chromium.org, dgozman@chromium.org
Looks good! Sorry, but I am not a Chromium committer - so you'll also have to wait for the review from somebody else on a DevTool team. https://codereview.chromium.org/2902673003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/resources/IndexedDBModel.js (right): https://codereview.chromium.org/2902673003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/resources/IndexedDBModel.js:196: refreshDatabaseView(databaseId) { "refreshDatabase"? https://codereview.chromium.org/2902673003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/resources/IndexedDBModel.js:198: Resources.IndexedDBModel.Events.DatabaseRefreshed, {model: this, databaseId: databaseId}); Don't pass model here - just the databaseId.
Thanks Eugene! https://codereview.chromium.org/2902673003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/resources/IndexedDBModel.js (right): https://codereview.chromium.org/2902673003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/resources/IndexedDBModel.js:196: refreshDatabaseView(databaseId) { On 2017/05/23 20:29:55, eostroukhov wrote: > "refreshDatabase"? Using refreshDatabase causes the tab to crash (there's probably some conflict but I'm not sure where). So I used a new function to only reload the views. https://codereview.chromium.org/2902673003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/resources/IndexedDBModel.js:198: Resources.IndexedDBModel.Events.DatabaseRefreshed, {model: this, databaseId: databaseId}); On 2017/05/23 20:29:55, eostroukhov wrote: > Don't pass model here - just the databaseId. Done.
https://codereview.chromium.org/2902673003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/resources/IndexedDBModel.js (right): https://codereview.chromium.org/2902673003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/resources/IndexedDBModel.js:196: refreshDatabaseView(databaseId) { On 2017/05/23 20:53:58, kristipark wrote: > On 2017/05/23 20:29:55, eostroukhov wrote: > > "refreshDatabase"? > > Using refreshDatabase causes the tab to crash (there's probably some conflict > but I'm not sure where). So I used a new function to only reload the views. There shouldn't be any crashes - can you open a bug and investigate what causes the problem?
initial comments https://codereview.chromium.org/2902673003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/inspector/indexeddb/database-refresh-view.html (right): https://codereview.chromium.org/2902673003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/indexeddb/database-refresh-view.html:15: function createIndexedDB(callback) { See style guide: https://google.github.io/styleguide/javascriptguide.xml#Function_Declarations... Please make the function a variable. https://codereview.chromium.org/2902673003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/indexeddb/database-refresh-view.html:43: var objectStoreTreeElement = UI.panels.resources._sidebar.indexedDBListTreeElement.childAt(0).childAt(0); Can you iterate each object store? https://codereview.chromium.org/2902673003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/indexeddb/database-refresh-view.html:72: InspectorTest.addResult("Added objectstore entry."); Can you also add another object store? - This might mean you change how you're doing promises - consider having the function call return another function so you can specify the name: var createObjectStore = function(name) { return function(resolve) { var mainFrameId = InspectorTest.resourceTreeModel.mainFrame.id; InspectorTest.createObjectStore(mainFrameId, databaseName, name, "key", true, resolve); } } Then you can check that the object store tree list changes.
On 2017/05/23 20:57:32, eostroukhov wrote: > https://codereview.chromium.org/2902673003/diff/1/third_party/WebKit/Source/d... > File third_party/WebKit/Source/devtools/front_end/resources/IndexedDBModel.js > (right): > > https://codereview.chromium.org/2902673003/diff/1/third_party/WebKit/Source/d... > third_party/WebKit/Source/devtools/front_end/resources/IndexedDBModel.js:196: > refreshDatabaseView(databaseId) { > On 2017/05/23 20:53:58, kristipark wrote: > > On 2017/05/23 20:29:55, eostroukhov wrote: > > > "refreshDatabase"? > > > > Using refreshDatabase causes the tab to crash (there's probably some conflict > > but I'm not sure where). So I used a new function to only reload the views. > > There shouldn't be any crashes - can you open a bug and investigate what causes > the problem? Hi Eugene, I took a look at it again, and the way I called refreshDatabase was the problem. Since I placed it in IDBDatabaseView._refreshDatabase(), it was being referenced every view update, which triggered another update and so on until the tab crashed. I changed it to only be called when the button was pressed and it worked perfectly. Sorry about that!
https://codereview.chromium.org/2902673003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/IndexedDBModel.js (right): https://codereview.chromium.org/2902673003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/IndexedDBModel.js:196: refreshDatabaseView(databaseId) { Should this actually be merged with method above? I don't see this method actually refreshing anything. Am I missing something?
Changed the refresh button to use the refreshDatabase() function instead of an event listener. Test now creates the database structure, still need to add entries to the datastores and check if refresh works. https://codereview.chromium.org/2902673003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/inspector/indexeddb/database-refresh-view.html (right): https://codereview.chromium.org/2902673003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/indexeddb/database-refresh-view.html:15: function createIndexedDB(callback) { On 2017/05/23 21:22:05, dmurph wrote: > See style guide: > https://google.github.io/styleguide/javascriptguide.xml#Function_Declarations... > > Please make the function a variable. Done. https://codereview.chromium.org/2902673003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/indexeddb/database-refresh-view.html:43: var objectStoreTreeElement = UI.panels.resources._sidebar.indexedDBListTreeElement.childAt(0).childAt(0); On 2017/05/23 21:22:05, dmurph wrote: > Can you iterate each object store? Done. https://codereview.chromium.org/2902673003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/indexeddb/database-refresh-view.html:72: InspectorTest.addResult("Added objectstore entry."); On 2017/05/23 21:22:05, dmurph wrote: > Can you also add another object store? - This might mean you change how you're > doing promises - consider having the function call return another function so > you can specify the name: > > var createObjectStore = function(name) { > return function(resolve) { > var mainFrameId = InspectorTest.resourceTreeModel.mainFrame.id; > InspectorTest.createObjectStore(mainFrameId, databaseName, name, "key", > true, resolve); > } > } > > Then you can check that the object store tree list changes. Done. https://codereview.chromium.org/2902673003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/IndexedDBModel.js (right): https://codereview.chromium.org/2902673003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/IndexedDBModel.js:196: refreshDatabaseView(databaseId) { On 2017/05/24 23:38:47, dgozman wrote: > Should this actually be merged with method above? I don't see this method > actually refreshing anything. Am I missing something? Yes, changed to use this method to refresh instead of the event listener.
https://codereview.chromium.org/2902673003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/inspector/indexeddb/database-refresh-view.html (right): https://codereview.chromium.org/2902673003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/indexeddb/database-refresh-view.html:9: var createDatabase = function(databaseName) { function createDatabase( https://codereview.chromium.org/2902673003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/indexeddb/database-refresh-view.html:23: var createObjectStore = function(databaseName, objectStoreName, indexName) { function createObjectStore( https://codereview.chromium.org/2902673003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/indexeddb/database-refresh-view.html:52: window.debugTest = true; Remove? https://codereview.chromium.org/2902673003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/indexeddb/database-refresh-view.html:65: var refreshDatabase = function(databaseId, callback) { function refreshDatabase( https://codereview.chromium.org/2902673003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/indexeddb/database-refresh-view.html:70: indexedDBModel.refreshDatabase(databaseId); It's better to get a view and emulate click on the button (e.g. by calling _refreshDatabaseButtonClicked) and then sniff for IDBDatabaseView._updatedForTest, which you call from update. This way you test the whole pipeline. update(database) { ... this._updatedForTest(); } _updatedForTest() { // Sniffed in tests. } https://codereview.chromium.org/2902673003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/indexeddb/database-refresh-view.html:78: var waitUntilAdded = function(callback) { function waitUntilAdded(callback) { https://codereview.chromium.org/2902673003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/inspector/indexeddb/indexeddb-test.js (right): https://codereview.chromium.org/2902673003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/indexeddb/indexeddb-test.js:26: for (var k = 0; k < objectStoreTreeElement.childCount(); ++k) { Nice fix! https://codereview.chromium.org/2902673003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/IndexedDBViews.js (right): https://codereview.chromium.org/2902673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/IndexedDBViews.js:71: this._refreshDatabase(); I think there is no need to call this, since model will emit DatabaseLoaded event after refresh and update(database) will be called.
Finished adding entries and testing if the refresh button refreshes the objectstore view. Last bit is to format the objectstore entry value into a string for the test output, but I'm not quite sure how to do that. Thanks for the review Dmitry! https://codereview.chromium.org/2902673003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/inspector/indexeddb/database-refresh-view.html (right): https://codereview.chromium.org/2902673003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/indexeddb/database-refresh-view.html:9: var createDatabase = function(databaseName) { On 2017/05/26 19:10:46, dgozman wrote: > function createDatabase( Done. https://codereview.chromium.org/2902673003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/indexeddb/database-refresh-view.html:23: var createObjectStore = function(databaseName, objectStoreName, indexName) { On 2017/05/26 19:10:46, dgozman wrote: > function createObjectStore( Done. https://codereview.chromium.org/2902673003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/indexeddb/database-refresh-view.html:52: window.debugTest = true; On 2017/05/26 19:10:46, dgozman wrote: > Remove? Done. https://codereview.chromium.org/2902673003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/indexeddb/database-refresh-view.html:65: var refreshDatabase = function(databaseId, callback) { On 2017/05/26 19:10:46, dgozman wrote: > function refreshDatabase( Done. https://codereview.chromium.org/2902673003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/indexeddb/database-refresh-view.html:70: indexedDBModel.refreshDatabase(databaseId); On 2017/05/26 19:10:46, dgozman wrote: > It's better to get a view and emulate click on the button (e.g. by calling > _refreshDatabaseButtonClicked) and then sniff for > IDBDatabaseView._updatedForTest, which you call from update. This way you test > the whole pipeline. > > update(database) { > ... > this._updatedForTest(); > } > > _updatedForTest() { > // Sniffed in tests. > } Done. https://codereview.chromium.org/2902673003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/indexeddb/database-refresh-view.html:78: var waitUntilAdded = function(callback) { On 2017/05/26 19:10:46, dgozman wrote: > function waitUntilAdded(callback) { Done. https://codereview.chromium.org/2902673003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/IndexedDBViews.js (right): https://codereview.chromium.org/2902673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/IndexedDBViews.js:71: this._refreshDatabase(); On 2017/05/26 19:10:46, dgozman wrote: > I think there is no need to call this, since model will emit DatabaseLoaded > event after refresh and update(database) will be called. Done.
The CQ bit was checked by kristipark@chromium.org
The CQ bit was unchecked by kristipark@chromium.org
The CQ bit was checked by kristipark@chromium.org
The CQ bit was unchecked by kristipark@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
lgtm. Nice work!
The CQ bit was checked by kristipark@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by kristipark@chromium.org
The CQ bit was checked by kristipark@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org Link to the patchset: https://codereview.chromium.org/2902673003/#ps80001 (title: "Removed a comment.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Removed a comment
Description was changed from ========== Added a refresh database button on the IndexedDB view. BUG=723871 R=dmurph@chromium.org, eostroukhov@chromium.org ========== to ========== Added a refresh database button on the IndexedDB view. BUG=723871 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by kristipark@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org Link to the patchset: https://codereview.chromium.org/2902673003/#ps100001 (title: "Fixing trybot failures, wrong security origin in test expected file.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm :)
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1496187292500560,
"parent_rev": "394848886c74b46a456a5eec488a9866b8f86b54", "commit_rev":
"075f218d5fc98ed1738637f05354ce9a3b0cb831"}
Message was sent while issue was closed.
Description was changed from ========== Added a refresh database button on the IndexedDB view. BUG=723871 ========== to ========== Added a refresh database button on the IndexedDB view. BUG=723871 Review-Url: https://codereview.chromium.org/2902673003 Cr-Commit-Position: refs/heads/master@{#475739} Committed: https://chromium.googlesource.com/chromium/src/+/075f218d5fc98ed1738637f05354... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/075f218d5fc98ed1738637f05354... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
