Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
677213002:
Add ImportHistory class and RecordStorage class. (Closed)
|
Created:
6 years, 2 months ago by Steve McKay Modified:
6 years, 1 month ago CC:
chromium-reviews, extensions-reviews_chromium.org, nkostylev+watch_chromium.org, tfarina, yoshiki+watch_chromium.org, rginda+watch_chromium.org, mtomasz+watch_chromium.org, oshima+watch_chromium.org, chromium-apps-reviews_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, Ben Kwa Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdd ImportHistory class and RecordStorage class to manage storage of local dedupe data.
RecordStorage will storage data in a file obtained from chrome.syncFileSystem allowing "local" dedupe data to be synced between a users devices.
BUG=420680
TEST=browser_test: *ImportHistoryTest*
Committed: https://crrev.com/2dc842c7481b6cc806bcd7b8cd027e496853149f
Cr-Commit-Position: refs/heads/master@{#302116}
Patch Set 1 #Patch Set 2 : Revert json file from (abandoned) browser test. #Patch Set 3 : Nuke an obsolete comment. #
Total comments: 52
Patch Set 4 : Respond to review comments && extract interface from RecordStorage. #
Total comments: 10
Patch Set 5 : Respond to review comments. #Patch Set 6 : Remove underscores from test method names. #
Total comments: 12
Patch Set 7 : Respond to Ben's comments. #Patch Set 8 : Address an outstanding TODO #
Total comments: 18
Patch Set 9 : Respond to review comments. #
Total comments: 14
Patch Set 10 : Respond to review comments. #
Total comments: 2
Patch Set 11 : Fix a comment typo. #
Messages
Total messages: 30 (5 generated)
smckay@chromium.org changed reviewers: + mtomasz@chromium.org
Whew! Really wanted to get this mailed before I went home for the weekend. Too bad I wasn't able to beat the weekend in Tokyo. This change is just one piece of the import pipeline. Consider checking out the dedupe section of the design doc for a refresher if this all seems a bit context-less.
https://codereview.chromium.org/677213002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_manager/file_manager_jstest.cc (right): https://codereview.chromium.org/677213002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/file_manager/file_manager_jstest.cc:64: IN_PROC_BROWSER_TEST_F(FileManagerJsTest, ImportHistoryTest) { Please add TEST=browser_test: *ImportHistoryTest* or sth like that to the CL description. https://codereview.chromium.org/677213002/diff/40001/chrome/test/data/file_ma... File chrome/test/data/file_manager/unit_tests/import_history_unittest.js (right): https://codereview.chromium.org/677213002/diff/40001/chrome/test/data/file_ma... chrome/test/data/file_manager/unit_tests/import_history_unittest.js:12: /** @type {!TestFileSystem} */ nit: The value is undefined, but jsdoc says it can't. https://codereview.chromium.org/677213002/diff/40001/chrome/test/data/file_ma... chrome/test/data/file_manager/unit_tests/import_history_unittest.js:21: var historyLoader; nit: jsdoc seems missing. https://codereview.chromium.org/677213002/diff/40001/chrome/test/data/file_ma... chrome/test/data/file_manager/unit_tests/import_history_unittest.js:24: // var entry; nit: Shall we uncomment? https://codereview.chromium.org/677213002/diff/40001/chrome/test/data/file_ma... chrome/test/data/file_manager/unit_tests/import_history_unittest.js:45: this.records_ = [ Please add jsdoc to members and methods. https://codereview.chromium.org/677213002/diff/40001/chrome/test/data/file_ma... chrome/test/data/file_manager/unit_tests/import_history_unittest.js:67: var onFsReady = function(fs) { nit: Avoid abbreviations per our style guide. How about simply fileSystem? https://codereview.chromium.org/677213002/diff/40001/chrome/test/data/file_ma... chrome/test/data/file_manager/unit_tests/import_history_unittest.js:105: function testHistory_NotImported(callback) { I'm not sure if _ in method names doesn't violate our style guide. Please check other tests if we do that. https://codereview.chromium.org/677213002/diff/40001/chrome/test/data/file_ma... chrome/test/data/file_manager/unit_tests/import_history_unittest.js:110: testHistory.wasImported(testFileEntry, DROPBOX).then( nit: Can we use some other fake name? https://codereview.chromium.org/677213002/diff/40001/chrome/test/data/file_ma... chrome/test/data/file_manager/unit_tests/import_history_unittest.js:133: testHistory.markImported(testFileEntry, 'Dropbox').then( nit: Same here. https://codereview.chromium.org/677213002/diff/40001/ui/file_manager/file_man... File ui/file_manager/file_manager/background/js/import_history.js (right): https://codereview.chromium.org/677213002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/import_history.js:10: * @param {!RecordStorage} storage nit: @param description is missing. https://codereview.chromium.org/677213002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/import_history.js:14: /** @private {!RecordStorage} */ @private -> @type...\n@private https://codereview.chromium.org/677213002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/import_history.js:17: /** @private {!Object.<string, !Array.<string>>} */ ditto https://codereview.chromium.org/677213002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/import_history.js:20: nit: We don't really use \n\n between methods in Chromium code. https://codereview.chromium.org/677213002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/import_history.js:56: console.log('Error', JSON.stringify(e)); In Files app code we use in Promise catch: console.error(error.stack || error). https://codereview.chromium.org/677213002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/import_history.js:63: * @param {string} key nit: Descriptions missing. https://codereview.chromium.org/677213002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/import_history.js:88: function(e) { nit: e -> error.
Excellent! Mostly Dones, and some questions about Google3 style VS team style and how those are supposed to get resolved. https://codereview.chromium.org/677213002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_manager/file_manager_jstest.cc (right): https://codereview.chromium.org/677213002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/file_manager/file_manager_jstest.cc:64: IN_PROC_BROWSER_TEST_F(FileManagerJsTest, ImportHistoryTest) { On 2014/10/27 00:40:11, mtomasz wrote: > Please add TEST=browser_test: *ImportHistoryTest* or sth like that to the CL > description. Done. Q: What's the background on that? Does it serve a functional role, or just informative? https://codereview.chromium.org/677213002/diff/40001/chrome/test/data/file_ma... File chrome/test/data/file_manager/unit_tests/import_history_unittest.js (right): https://codereview.chromium.org/677213002/diff/40001/chrome/test/data/file_ma... chrome/test/data/file_manager/unit_tests/import_history_unittest.js:12: /** @type {!TestFileSystem} */ On 2014/10/27 00:40:11, mtomasz wrote: > nit: The value is undefined, but jsdoc says it can't. So I read this as "this thing can't ever be null". Based on experience with our internal compiled tests it can start out undefined without any compiler complaints. But you can't then assign `undefined` to it unless it is declared {!TestFileSystem|undefined}. So I think this is the strictest way to declare the var. https://codereview.chromium.org/677213002/diff/40001/chrome/test/data/file_ma... chrome/test/data/file_manager/unit_tests/import_history_unittest.js:21: var historyLoader; On 2014/10/27 00:40:11, mtomasz wrote: > nit: jsdoc seems missing. Done. https://codereview.chromium.org/677213002/diff/40001/chrome/test/data/file_ma... chrome/test/data/file_manager/unit_tests/import_history_unittest.js:24: // var entry; On 2014/10/27 00:40:12, mtomasz wrote: > nit: Shall we uncomment? Nuked. https://codereview.chromium.org/677213002/diff/40001/chrome/test/data/file_ma... chrome/test/data/file_manager/unit_tests/import_history_unittest.js:45: this.records_ = [ On 2014/10/27 00:40:11, mtomasz wrote: > Please add jsdoc to members and methods. Let's call this done. Here's what I did and why. Lemme know if you're happy with this appoach. If so, we'll stick to this pattern for future CLs. THE WHY: It sounds like the team is quickly moving towards using the closure compiler on the source. THE WHAT: I introduced a RecordStorage interface, and renamed the existing implementation to FileEntryRecordStorage. That'll let use easily substitute the test double in the tests, even when they are compiled. https://codereview.chromium.org/677213002/diff/40001/chrome/test/data/file_ma... chrome/test/data/file_manager/unit_tests/import_history_unittest.js:67: var onFsReady = function(fs) { On 2014/10/27 00:40:11, mtomasz wrote: > nit: Avoid abbreviations per our style guide. How about simply fileSystem? Done. https://codereview.chromium.org/677213002/diff/40001/chrome/test/data/file_ma... chrome/test/data/file_manager/unit_tests/import_history_unittest.js:105: function testHistory_NotImported(callback) { On 2014/10/27 00:40:11, mtomasz wrote: > I'm not sure if _ in method names doesn't violate our style guide. Please check > other tests if we do that. It's permitted in Google style (internal) for basically qualifications of test cases. I don't, however see it used in the immediate vicinity of this test. I'd like to make the argument that this type of qualification can aid readability of tests by allowing authors to pack details into a method name, while retaining some reader-beneficial segmentation. If you still don't like, I'll change. But at least consider the benefits. https://codereview.chromium.org/677213002/diff/40001/chrome/test/data/file_ma... chrome/test/data/file_manager/unit_tests/import_history_unittest.js:110: testHistory.wasImported(testFileEntry, DROPBOX).then( On 2014/10/27 00:40:11, mtomasz wrote: > nit: Can we use some other fake name? Good point. Done..."Space Cloud" :) https://codereview.chromium.org/677213002/diff/40001/chrome/test/data/file_ma... chrome/test/data/file_manager/unit_tests/import_history_unittest.js:133: testHistory.markImported(testFileEntry, 'Dropbox').then( On 2014/10/27 00:40:11, mtomasz wrote: > nit: Same here. Done. https://codereview.chromium.org/677213002/diff/40001/ui/file_manager/file_man... File ui/file_manager/file_manager/background/js/import_history.js (right): https://codereview.chromium.org/677213002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/import_history.js:10: * @param {!RecordStorage} storage On 2014/10/27 00:40:12, mtomasz wrote: > nit: @param description is missing. Descriptions are optional in google3 JS rules, especially if it'll be an obligatory comment. E.g. "The storage that holds records." In this case, the docs on the interface should be sufficient. I guess the question is: Which style rules are we following? And if we're not following google3 rules specifically, is it reasonable to invoke them? Lemme know what you think. https://codereview.chromium.org/677213002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/import_history.js:14: /** @private {!RecordStorage} */ On 2014/10/27 00:40:12, mtomasz wrote: > @private -> @type...\n@private The @type is inferred, Closure totally supports that and it is the norm in new Google3 code. I think there are some other short cut's, but this has been our favorite. I see the public docs don't mention this. Checking with my Closure buddy (tbreisacher@) to see if that behavior is documented publicly somewhere. https://codereview.chromium.org/677213002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/import_history.js:17: /** @private {!Object.<string, !Array.<string>>} */ On 2014/10/27 00:40:12, mtomasz wrote: > ditto Acknowledged. https://codereview.chromium.org/677213002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/import_history.js:20: On 2014/10/27 00:40:12, mtomasz wrote: > nit: We don't really use \n\n between methods in Chromium code. Done. I've never really liked the whitespace rules for JS :) https://codereview.chromium.org/677213002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/import_history.js:56: console.log('Error', JSON.stringify(e)); On 2014/10/27 00:40:12, mtomasz wrote: > In Files app code we use in Promise catch: > console.error(error.stack || error). Ah, yes. I meant to ask about best practices in error handling. I still don't fully grok error handling in Promises. Sounds like you're okay with this idiom. https://codereview.chromium.org/677213002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/import_history.js:63: * @param {string} key On 2014/10/27 00:40:12, mtomasz wrote: > nit: Descriptions missing. I'd say this is one of those that can be skipped--private method and key is opaque (unless you peek in the getKey_ method). How does the team usually make style guide decisions? Should I open up the discussion on the chromeos-drive list? In the end, I'll make what ever changes the team deems appropriate, but I'd at least like to advocate from my perspective first. https://codereview.chromium.org/677213002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/import_history.js:88: function(e) { On 2014/10/27 00:40:12, mtomasz wrote: > nit: e -> error. Done.
mtomasz@chromium.org changed reviewers: + fukino@chromium.org
Yes, code style of JS in Chromium, especially jsdoc is not so well defined. I added @fukino who works on making our code compile with Closure compiler. @fukino: What do you think? https://codereview.chromium.org/677213002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_manager/file_manager_jstest.cc (right): https://codereview.chromium.org/677213002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/file_manager/file_manager_jstest.cc:64: IN_PROC_BROWSER_TEST_F(FileManagerJsTest, ImportHistoryTest) { On 2014/10/27 21:06:28, Steve McKay wrote: > On 2014/10/27 00:40:11, mtomasz wrote: > > Please add TEST=browser_test: *ImportHistoryTest* or sth like that to the CL > > description. > > Done. > > Q: What's the background on that? Does it serve a functional role, or just > informative? Good question. I was asked by testers to write a way to verify each CL. If it's automatically tested, then they would like to know about it. But I'm not really sure if it's a hard rule. https://codereview.chromium.org/677213002/diff/40001/chrome/test/data/file_ma... File chrome/test/data/file_manager/unit_tests/import_history_unittest.js (right): https://codereview.chromium.org/677213002/diff/40001/chrome/test/data/file_ma... chrome/test/data/file_manager/unit_tests/import_history_unittest.js:12: /** @type {!TestFileSystem} */ On 2014/10/27 21:06:29, Steve McKay wrote: > On 2014/10/27 00:40:11, mtomasz wrote: > > nit: The value is undefined, but jsdoc says it can't. > > So I read this as "this thing can't ever be null". Based on experience with our > internal compiled tests it can start out undefined without any compiler > complaints. But you can't then assign `undefined` to it unless it is declared > {!TestFileSystem|undefined}. > > So I think this is the strictest way to declare the var. Hm. I'm not really sure, we started using Closure compiler very recently in Files app. Added @fukino who is an expert on closure compiling. What I'm concerned it that you may pass testFileSystem to a function which accepts {!TestFileSystem}. Closure compiler wouldn't complain because jsdoc matches, but we will end up on passing undefined to the function. Correct me if my understanding is wrong. https://codereview.chromium.org/677213002/diff/40001/chrome/test/data/file_ma... chrome/test/data/file_manager/unit_tests/import_history_unittest.js:45: this.records_ = [ On 2014/10/27 21:06:29, Steve McKay wrote: > On 2014/10/27 00:40:11, mtomasz wrote: > > Please add jsdoc to members and methods. > > Let's call this done. Here's what I did and why. Lemme know if you're happy with > this appoach. If so, we'll stick to this pattern for future CLs. > > THE WHY: > > It sounds like the team is quickly moving towards using the closure compiler on > the source. > > THE WHAT: > > I introduced a RecordStorage interface, and renamed the existing implementation > to FileEntryRecordStorage. That'll let use easily substitute the test double in > the tests, even when they are compiled. > SGTM https://codereview.chromium.org/677213002/diff/40001/chrome/test/data/file_ma... chrome/test/data/file_manager/unit_tests/import_history_unittest.js:105: function testHistory_NotImported(callback) { On 2014/10/27 21:06:29, Steve McKay wrote: > On 2014/10/27 00:40:11, mtomasz wrote: > > I'm not sure if _ in method names doesn't violate our style guide. Please > check > > other tests if we do that. > > It's permitted in Google style (internal) for basically qualifications of test > cases. I don't, however see it used in the immediate vicinity of this test. > > I'd like to make the argument that this type of qualification can aid > readability of tests by allowing authors to pack details into a method name, > while retaining some reader-beneficial segmentation. > > If you still don't like, I'll change. But at least consider the benefits. It makes sense from the readability point of view, but our style guide doesn't have this rule. By the way do we need the prefix, if it's the same for all test cases in this file? https://codereview.chromium.org/677213002/diff/40001/ui/file_manager/file_man... File ui/file_manager/file_manager/background/js/import_history.js (right): https://codereview.chromium.org/677213002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/import_history.js:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. nit: Copyright (c) 2014 -> Copyright 2014 https://codereview.chromium.org/677213002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/import_history.js:10: * @param {!RecordStorage} storage On 2014/10/27 21:06:29, Steve McKay wrote: > On 2014/10/27 00:40:12, mtomasz wrote: > > nit: @param description is missing. > > Descriptions are optional in google3 JS rules, especially if it'll be an > obligatory comment. E.g. "The storage that holds records." > > In this case, the docs on the interface should be sufficient. > > I guess the question is: Which style rules are we following? And if we're not > following google3 rules specifically, is it reasonable to invoke them? > > Lemme know what you think. Basically in Chromium we don't follow google3 style, but the public one: http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml https://developers.google.com/closure/compiler/docs/js-for-compiler Deferring the decision to @fukino who works on migrating our jsdoc to Closure compiler compatible format. https://codereview.chromium.org/677213002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/import_history.js:14: /** @private {!RecordStorage} */ On 2014/10/27 21:06:29, Steve McKay wrote: > On 2014/10/27 00:40:12, mtomasz wrote: > > @private -> @type...\n@private > > The @type is inferred, Closure totally supports that and it is the norm in new > Google3 code. I think there are some other short cut's, but this has been our > favorite. > > I see the public docs don't mention this. Checking with my Closure buddy > (tbreisacher@) to see if that behavior is documented publicly somewhere. You're right. This seems to be accepted by our style guide. Feel free to use it. http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml https://codereview.chromium.org/677213002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/import_history.js:56: console.log('Error', JSON.stringify(e)); On 2014/10/27 21:06:29, Steve McKay wrote: > On 2014/10/27 00:40:12, mtomasz wrote: > > In Files app code we use in Promise catch: > > console.error(error.stack || error). > > Ah, yes. I meant to ask about best practices in error handling. I still don't > fully grok error handling in Promises. > > Sounds like you're okay with this idiom. If you find out the perfect way to handle errors in promises, let me know :). https://codereview.chromium.org/677213002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/import_history.js:63: * @param {string} key On 2014/10/27 21:06:29, Steve McKay wrote: > On 2014/10/27 00:40:12, mtomasz wrote: > > nit: Descriptions missing. > > I'd say this is one of those that can be skipped--private method and key is > opaque (unless you peek in the getKey_ method). How does the team usually make > style guide decisions? Should I open up the discussion on the chromeos-drive > list? > > In the end, I'll make what ever changes the team deems appropriate, but I'd at > least like to advocate from my perspective first. We used to write descriptions for each @param as the style guide (IIRC) doesn't clearly say that they are optional. I agree they are sometimes redundant. +@fukino for his opinion on this.
https://codereview.chromium.org/677213002/diff/40001/chrome/test/data/file_ma... File chrome/test/data/file_manager/unit_tests/import_history_unittest.js (right): https://codereview.chromium.org/677213002/diff/40001/chrome/test/data/file_ma... chrome/test/data/file_manager/unit_tests/import_history_unittest.js:12: /** @type {!TestFileSystem} */ On 2014/10/28 01:36:32, mtomasz wrote: > On 2014/10/27 21:06:29, Steve McKay wrote: > > On 2014/10/27 00:40:11, mtomasz wrote: > > > nit: The value is undefined, but jsdoc says it can't. > > > > So I read this as "this thing can't ever be null". Based on experience with > our > > internal compiled tests it can start out undefined without any compiler > > complaints. But you can't then assign `undefined` to it unless it is declared > > {!TestFileSystem|undefined}. > > > > So I think this is the strictest way to declare the var. > > Hm. I'm not really sure, we started using Closure compiler very recently in > Files app. Added @fukino who is an expert on closure compiling. > > What I'm concerned it that you may pass testFileSystem to a function which > accepts {!TestFileSystem}. Closure compiler wouldn't complain because jsdoc > matches, but we will end up on passing undefined to the function. Correct me if > my understanding is wrong. We use the style /** @type {!Foo} */ var foo; in externs files when the foo is already initialized in the global object. But, in this case, I have the same concern with mtomasz. If the foo is initialized later in this file, {!Foo|undefined} makes more sense to me. https://codereview.chromium.org/677213002/diff/40001/ui/file_manager/file_man... File ui/file_manager/file_manager/background/js/import_history.js (right): https://codereview.chromium.org/677213002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/import_history.js:63: * @param {string} key On 2014/10/28 01:36:32, mtomasz wrote: > On 2014/10/27 21:06:29, Steve McKay wrote: > > On 2014/10/27 00:40:12, mtomasz wrote: > > > nit: Descriptions missing. > > > > I'd say this is one of those that can be skipped--private method and key is > > opaque (unless you peek in the getKey_ method). How does the team usually make > > style guide decisions? Should I open up the discussion on the chromeos-drive > > list? > > > > In the end, I'll make what ever changes the team deems appropriate, but I'd at > > least like to advocate from my perspective first. > > We used to write descriptions for each @param as the style guide (IIRC) doesn't > clearly say that they are optional. I agree they are sometimes redundant. > +@fukino for his opinion on this. I think we can omit descriptions for each @param when they look redundant. In sample code in the style guide, descriptions for @param are sometimes omitted. I think we can follow the style.
https://codereview.chromium.org/677213002/diff/60001/chrome/test/data/file_ma... File chrome/test/data/file_manager/unit_tests/import_history_unittest.js (right): https://codereview.chromium.org/677213002/diff/60001/chrome/test/data/file_ma... chrome/test/data/file_manager/unit_tests/import_history_unittest.js:11: * @type {String} nit: {String} -> {string} for consistency with #6? https://codereview.chromium.org/677213002/diff/60001/chrome/test/data/file_ma... chrome/test/data/file_manager/unit_tests/import_history_unittest.js:11: * @type {String} nit: Shall it be @const? https://codereview.chromium.org/677213002/diff/60001/chrome/test/data/file_ma... chrome/test/data/file_manager/unit_tests/import_history_unittest.js:75: var onFsReady = function(fileSystem) { nit: Fs -> FileSystem https://codereview.chromium.org/677213002/diff/60001/chrome/test/data/file_ma... chrome/test/data/file_manager/unit_tests/import_history_unittest.js:111: console.log(error.stacktrace || error); nit: Can we replace it with console.error as it's an error? Also, shall stacktrace be stack? Per new Error().stack vs. new Error().stacktrace. https://codereview.chromium.org/677213002/diff/60001/ui/file_manager/file_man... File ui/file_manager/file_manager/background/js/import_history.js (right): https://codereview.chromium.org/677213002/diff/60001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/import_history.js:136: nit: \n\n\n -> \n?
Patchset #5 (id:80001) has been deleted
K. I think I covered all the comments. https://codereview.chromium.org/677213002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_manager/file_manager_jstest.cc (right): https://codereview.chromium.org/677213002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/file_manager/file_manager_jstest.cc:64: IN_PROC_BROWSER_TEST_F(FileManagerJsTest, ImportHistoryTest) { On 2014/10/28 01:36:32, mtomasz wrote: > On 2014/10/27 21:06:28, Steve McKay wrote: > > On 2014/10/27 00:40:11, mtomasz wrote: > > > Please add TEST=browser_test: *ImportHistoryTest* or sth like that to the CL > > > description. > > > > Done. > > > > Q: What's the background on that? Does it serve a functional role, or just > > informative? > > Good question. I was asked by testers to write a way to verify each CL. If it's > automatically tested, then they would like to know about it. But I'm not really > sure if it's a hard rule. I don't mind. Just wondering. https://codereview.chromium.org/677213002/diff/40001/chrome/test/data/file_ma... File chrome/test/data/file_manager/unit_tests/import_history_unittest.js (right): https://codereview.chromium.org/677213002/diff/40001/chrome/test/data/file_ma... chrome/test/data/file_manager/unit_tests/import_history_unittest.js:12: /** @type {!TestFileSystem} */ On 2014/10/28 04:05:59, fukino wrote: > On 2014/10/28 01:36:32, mtomasz wrote: > > On 2014/10/27 21:06:29, Steve McKay wrote: > > > On 2014/10/27 00:40:11, mtomasz wrote: > > > > nit: The value is undefined, but jsdoc says it can't. > > > > > > So I read this as "this thing can't ever be null". Based on experience with > > our > > > internal compiled tests it can start out undefined without any compiler > > > complaints. But you can't then assign `undefined` to it unless it is > declared > > > {!TestFileSystem|undefined}. > > > > > > So I think this is the strictest way to declare the var. > > > > Hm. I'm not really sure, we started using Closure compiler very recently in > > Files app. Added @fukino who is an expert on closure compiling. > > > > What I'm concerned it that you may pass testFileSystem to a function which > > accepts {!TestFileSystem}. Closure compiler wouldn't complain because jsdoc > > matches, but we will end up on passing undefined to the function. Correct me > if > > my understanding is wrong. > > We use the style > /** @type {!Foo} */ > var foo; > in externs files when the foo is already initialized in the global object. > But, in this case, I have the same concern with mtomasz. > If the foo is initialized later in this file, {!Foo|undefined} makes more sense > to me. > Done. But respectfully under protest. https://codereview.chromium.org/677213002/diff/40001/chrome/test/data/file_ma... chrome/test/data/file_manager/unit_tests/import_history_unittest.js:45: this.records_ = [ On 2014/10/28 01:36:32, mtomasz wrote: > On 2014/10/27 21:06:29, Steve McKay wrote: > > On 2014/10/27 00:40:11, mtomasz wrote: > > > Please add jsdoc to members and methods. > > > > Let's call this done. Here's what I did and why. Lemme know if you're happy > with > > this appoach. If so, we'll stick to this pattern for future CLs. > > > > THE WHY: > > > > It sounds like the team is quickly moving towards using the closure compiler > on > > the source. > > > > THE WHAT: > > > > I introduced a RecordStorage interface, and renamed the existing > implementation > > to FileEntryRecordStorage. That'll let use easily substitute the test double > in > > the tests, even when they are compiled. > > > > SGTM Acknowledged. https://codereview.chromium.org/677213002/diff/40001/chrome/test/data/file_ma... chrome/test/data/file_manager/unit_tests/import_history_unittest.js:105: function testHistory_NotImported(callback) { On 2014/10/28 01:36:32, mtomasz wrote: > On 2014/10/27 21:06:29, Steve McKay wrote: > > On 2014/10/27 00:40:11, mtomasz wrote: > > > I'm not sure if _ in method names doesn't violate our style guide. Please > > check > > > other tests if we do that. > > > > It's permitted in Google style (internal) for basically qualifications of test > > cases. I don't, however see it used in the immediate vicinity of this test. > > > > I'd like to make the argument that this type of qualification can aid > > readability of tests by allowing authors to pack details into a method name, > > while retaining some reader-beneficial segmentation. > > > > If you still don't like, I'll change. But at least consider the benefits. > > It makes sense from the readability point of view, but our style guide doesn't > have this rule. By the way do we need the prefix, if it's the same for all test > cases in this file? Okay. Nuked the underscores, but under protest...respectfully. testRecordWriter (now renamed testRecordStorage) doesn't share the same testHistory prefix. What's going on here is I'm cramming unit tests for two separate classes into this one file. Usually it would be 1-to-1, and one class per file, but that doesn't seem to be the norm in this codebase. Trying to stick to the norms right now. https://codereview.chromium.org/677213002/diff/40001/ui/file_manager/file_man... File ui/file_manager/file_manager/background/js/import_history.js (right): https://codereview.chromium.org/677213002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/import_history.js:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/10/28 01:36:32, mtomasz wrote: > nit: Copyright (c) 2014 -> Copyright 2014 Done. https://codereview.chromium.org/677213002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/import_history.js:10: * @param {!RecordStorage} storage On 2014/10/28 01:36:32, mtomasz wrote: > On 2014/10/27 21:06:29, Steve McKay wrote: > > On 2014/10/27 00:40:12, mtomasz wrote: > > > nit: @param description is missing. > > > > Descriptions are optional in google3 JS rules, especially if it'll be an > > obligatory comment. E.g. "The storage that holds records." > > > > In this case, the docs on the interface should be sufficient. > > > > I guess the question is: Which style rules are we following? And if we're not > > following google3 rules specifically, is it reasonable to invoke them? > > > > Lemme know what you think. > > Basically in Chromium we don't follow google3 style, but the public one: > http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml > https://developers.google.com/closure/compiler/docs/js-for-compiler > > Deferring the decision to @fukino who works on migrating our jsdoc to Closure > compiler compatible format. Acknowledged. https://codereview.chromium.org/677213002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/import_history.js:14: /** @private {!RecordStorage} */ On 2014/10/28 01:36:32, mtomasz wrote: > On 2014/10/27 21:06:29, Steve McKay wrote: > > On 2014/10/27 00:40:12, mtomasz wrote: > > > @private -> @type...\n@private > > > > The @type is inferred, Closure totally supports that and it is the norm in new > > Google3 code. I think there are some other short cut's, but this has been our > > favorite. > > > > I see the public docs don't mention this. Checking with my Closure buddy > > (tbreisacher@) to see if that behavior is documented publicly somewhere. > > You're right. This seems to be accepted by our style guide. Feel free to use it. > http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml I think you'll like it too. Closure is very verbose. It's nice to save some extra typing when possible. https://codereview.chromium.org/677213002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/import_history.js:56: console.log('Error', JSON.stringify(e)); On 2014/10/28 01:36:32, mtomasz wrote: > On 2014/10/27 21:06:29, Steve McKay wrote: > > On 2014/10/27 00:40:12, mtomasz wrote: > > > In Files app code we use in Promise catch: > > > console.error(error.stack || error). > > > > Ah, yes. I meant to ask about best practices in error handling. I still don't > > fully grok error handling in Promises. > > > > Sounds like you're okay with this idiom. > > If you find out the perfect way to handle errors in promises, let me know :). :/ Ben's been working on a best practices doc for Promises. I just asked him to share it with the team. But my understanding is that at the end of any chain of promises you should always put a .catch, else errors can (will) get swallowed, making it extraordinarily hard to debug problems. The one thing I know for sure, is in the beta build of Chrome, you can't just `@throw new Error...` from pretty much anywhere inside the promises and that to work. https://codereview.chromium.org/677213002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/import_history.js:63: * @param {string} key On 2014/10/28 04:05:59, fukino wrote: > On 2014/10/28 01:36:32, mtomasz wrote: > > On 2014/10/27 21:06:29, Steve McKay wrote: > > > On 2014/10/27 00:40:12, mtomasz wrote: > > > > nit: Descriptions missing. > > > > > > I'd say this is one of those that can be skipped--private method and key is > > > opaque (unless you peek in the getKey_ method). How does the team usually > make > > > style guide decisions? Should I open up the discussion on the chromeos-drive > > > list? > > > > > > In the end, I'll make what ever changes the team deems appropriate, but I'd > at > > > least like to advocate from my perspective first. > > > > We used to write descriptions for each @param as the style guide (IIRC) > doesn't > > clearly say that they are optional. I agree they are sometimes redundant. > > +@fukino for his opinion on this. > > I think we can omit descriptions for each @param when they look redundant. > In sample code in the style guide, descriptions for @param are sometimes > omitted. I think we can follow the style. Acknowledged. https://codereview.chromium.org/677213002/diff/60001/chrome/test/data/file_ma... File chrome/test/data/file_manager/unit_tests/import_history_unittest.js (right): https://codereview.chromium.org/677213002/diff/60001/chrome/test/data/file_ma... chrome/test/data/file_manager/unit_tests/import_history_unittest.js:11: * @type {String} On 2014/10/28 08:47:59, mtomasz wrote: > nit: Shall it be @const? Ditto. https://codereview.chromium.org/677213002/diff/60001/chrome/test/data/file_ma... chrome/test/data/file_manager/unit_tests/import_history_unittest.js:11: * @type {String} On 2014/10/28 08:47:59, mtomasz wrote: > nit: {String} -> {string} for consistency with #6? Woops. Good catch. https://codereview.chromium.org/677213002/diff/60001/chrome/test/data/file_ma... chrome/test/data/file_manager/unit_tests/import_history_unittest.js:75: var onFsReady = function(fileSystem) { On 2014/10/28 08:47:59, mtomasz wrote: > nit: Fs -> FileSystem Done. https://codereview.chromium.org/677213002/diff/60001/chrome/test/data/file_ma... chrome/test/data/file_manager/unit_tests/import_history_unittest.js:111: console.log(error.stacktrace || error); On 2014/10/28 08:47:59, mtomasz wrote: > nit: Can we replace it with console.error as it's an error? Also, shall > stacktrace be stack? Per new Error().stack vs. new Error().stacktrace. Mmm. Better. It's hard to look like a pro without a compiler catching my mistakes :) https://codereview.chromium.org/677213002/diff/60001/ui/file_manager/file_man... File ui/file_manager/file_manager/background/js/import_history.js (right): https://codereview.chromium.org/677213002/diff/60001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/import_history.js:136: On 2014/10/28 08:47:59, mtomasz wrote: > nit: \n\n\n -> \n? Woops. Force of habit. Okay. I think I nuked all extra lines.
kenobi@chromium.org changed reviewers: + kenobi@chromium.org
https://codereview.chromium.org/677213002/diff/120001/chrome/test/data/file_m... File chrome/test/data/file_manager/unit_tests/import_history_unittest.js (right): https://codereview.chromium.org/677213002/diff/120001/chrome/test/data/file_m... chrome/test/data/file_manager/unit_tests/import_history_unittest.js:10: * Space Cloud: You're source for interstellar cloud storage. nit: "Your" https://codereview.chromium.org/677213002/diff/120001/chrome/test/data/file_m... chrome/test/data/file_manager/unit_tests/import_history_unittest.js:96: createRealStorage().then( nit: personally, I find it more readable if all the then and catch clauses are at the same indent level. nit: style guide calls for the dot to be on the preceding line. doSomething(). then(doSomethingElse). catch(errorHandler); https://codereview.chromium.org/677213002/diff/120001/chrome/test/data/file_m... chrome/test/data/file_manager/unit_tests/import_history_unittest.js:160: function(error) { All of the error handlers in this file are doing the same thing. Can we factor that code? https://codereview.chromium.org/677213002/diff/120001/ui/file_manager/file_ma... File ui/file_manager/file_manager/background/js/import_history.js (right): https://codereview.chromium.org/677213002/diff/120001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/import_history.js:26: * @return {!Promise.<!ImportHistory>} Settles when the history object is ready. nit: "Resolves" or "fulfills". "Settles" means either resolved or rejected. Ditto the rest of the file. https://codereview.chromium.org/677213002/diff/120001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/import_history.js:234: reader.onloadend = function() { onloadend and onerror are event callbacks - this means onerror can be called (possibly multiple times) followed by onloadend. A promise can only be settled once, so I think the code in the "if (!!reader.error)" clause is redundant.
Addressed Ben's comments. PTAL. https://codereview.chromium.org/677213002/diff/120001/chrome/test/data/file_m... File chrome/test/data/file_manager/unit_tests/import_history_unittest.js (right): https://codereview.chromium.org/677213002/diff/120001/chrome/test/data/file_m... chrome/test/data/file_manager/unit_tests/import_history_unittest.js:10: * Space Cloud: You're source for interstellar cloud storage. On 2014/10/28 18:44:55, Ben Kwa wrote: > nit: "Your" Doh! Muscle memory. https://codereview.chromium.org/677213002/diff/120001/chrome/test/data/file_m... chrome/test/data/file_manager/unit_tests/import_history_unittest.js:96: createRealStorage().then( On 2014/10/28 18:44:55, Ben Kwa wrote: > nit: personally, I find it more readable if all the then and catch clauses are > at the same indent level. > nit: style guide calls for the dot to be on the preceding line. > > doSomething(). > then(doSomethingElse). > catch(errorHandler); Done...wrapping. But dots on next line. Linter allows both forms. https://codereview.chromium.org/677213002/diff/120001/chrome/test/data/file_m... chrome/test/data/file_manager/unit_tests/import_history_unittest.js:160: function(error) { On 2014/10/28 18:44:55, Ben Kwa wrote: > All of the error handlers in this file are doing the same thing. Can we factor > that code? Done. https://codereview.chromium.org/677213002/diff/120001/ui/file_manager/file_ma... File ui/file_manager/file_manager/background/js/import_history.js (right): https://codereview.chromium.org/677213002/diff/120001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/import_history.js:26: * @return {!Promise.<!ImportHistory>} Settles when the history object is ready. On 2014/10/28 18:44:55, Ben Kwa wrote: > nit: "Resolves" or "fulfills". "Settles" means either resolved or rejected. > Ditto the rest of the file. Done. https://codereview.chromium.org/677213002/diff/120001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/import_history.js:234: reader.onloadend = function() { On 2014/10/28 18:44:55, Ben Kwa wrote: > onloadend and onerror are event callbacks - this means onerror can be called > (possibly multiple times) followed by onloadend. A promise can only be settled > once, so I think the code in the "if (!!reader.error)" clause is redundant. I verified at runtime that the reader can have the .error field on it. So I'd like to consult it JIC...so I can deal with a failure to read predictably.
https://codereview.chromium.org/677213002/diff/160001/chrome/test/data/file_m... File chrome/test/data/file_manager/unit_tests/import_history_unittest.js (right): https://codereview.chromium.org/677213002/diff/160001/chrome/test/data/file_m... chrome/test/data/file_manager/unit_tests/import_history_unittest.js:34: modificationTime: 'Thursday, very late' nit: Can we use a Date object? https://codereview.chromium.org/677213002/diff/160001/chrome/test/data/file_m... chrome/test/data/file_manager/unit_tests/import_history_unittest.js:101: function testRecordStorage(callback) { nit: Shall we add jsdoc to these functions? https://codereview.chromium.org/677213002/diff/160001/chrome/test/data/file_m... chrome/test/data/file_manager/unit_tests/import_history_unittest.js:156: nit: \n\n -> \n https://codereview.chromium.org/677213002/diff/160001/chrome/test/data/file_m... chrome/test/data/file_manager/unit_tests/import_history_unittest.js:158: function handleError_(callback, error) { nit: Private in what scope? Maybe no need for _ at the end? nit: jsdoc missing nit: ; at the end redundant https://codereview.chromium.org/677213002/diff/160001/ui/file_manager/file_ma... File ui/file_manager/file_manager/background/js/import_history.js (right): https://codereview.chromium.org/677213002/diff/160001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/import_history.js:19: }; nit: ; seems redundant here. https://codereview.chromium.org/677213002/diff/160001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/import_history.js:160: RecordStorage.prototype.write; Will this compile? We used to write an empty method implementation in interfaces, like RS.prototype.write = function(record) {}. I'm fine with this, as long as it compiles. https://codereview.chromium.org/677213002/diff/160001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/import_history.js:213: {type: 'application/json; charset=utf-8'}); It's not really a valid json. How about simply plain/text mime type? https://codereview.chromium.org/677213002/diff/160001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/import_history.js:230: .then(this.readFileAsText_.bind(this), function() { return '';}) nit: space after ; missing. https://codereview.chromium.org/677213002/diff/160001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/import_history.js:291: resolve(JSON.parse(json)); Date objects will not deserialize properly from json. Shall we add a comment about it, or add a special logic for creating Date objects when deserializing?
https://codereview.chromium.org/677213002/diff/120001/ui/file_manager/file_ma... File ui/file_manager/file_manager/background/js/import_history.js (right): https://codereview.chromium.org/677213002/diff/120001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/import_history.js:234: reader.onloadend = function() { On 2014/10/28 19:32:19, Steve McKay wrote: > On 2014/10/28 18:44:55, Ben Kwa wrote: > > onloadend and onerror are event callbacks - this means onerror can be called > > (possibly multiple times) followed by onloadend. A promise can only be > settled > > once, so I think the code in the "if (!!reader.error)" clause is redundant. > > I verified at runtime that the reader can have the .error field on it. So I'd > like to consult it JIC...so I can deal with a failure to read predictably. Ok. If the situation you're describing occurs, then should line 246 reject rather than resolve? Maybe reject(reader.error)?
PTALs everyone. Hopefully we can land this change soon. We'll continue to work the code as we build out the integration and start testing our initial assumptions. https://codereview.chromium.org/677213002/diff/120001/ui/file_manager/file_ma... File ui/file_manager/file_manager/background/js/import_history.js (right): https://codereview.chromium.org/677213002/diff/120001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/import_history.js:234: reader.onloadend = function() { On 2014/10/29 13:29:51, Ben Kwa wrote: > On 2014/10/28 19:32:19, Steve McKay wrote: > > On 2014/10/28 18:44:55, Ben Kwa wrote: > > > onloadend and onerror are event callbacks - this means onerror can be called > > > (possibly multiple times) followed by onloadend. A promise can only be > > settled > > > once, so I think the code in the "if (!!reader.error)" clause is redundant. > > > > I verified at runtime that the reader can have the .error field on it. So I'd > > like to consult it JIC...so I can deal with a failure to read predictably (line 209). > > Ok. If the situation you're describing occurs, then should line 246 reject > rather than resolve? Maybe reject(reader.error)? Done. Also, report the error in the reject handler for this promise. https://codereview.chromium.org/677213002/diff/160001/chrome/test/data/file_m... File chrome/test/data/file_manager/unit_tests/import_history_unittest.js (right): https://codereview.chromium.org/677213002/diff/160001/chrome/test/data/file_m... chrome/test/data/file_manager/unit_tests/import_history_unittest.js:34: modificationTime: 'Thursday, very late' On 2014/10/29 01:10:40, mtomasz wrote: > nit: Can we use a Date object? Done (in a const), but the type is still string. https://codereview.chromium.org/677213002/diff/160001/chrome/test/data/file_m... chrome/test/data/file_manager/unit_tests/import_history_unittest.js:101: function testRecordStorage(callback) { On 2014/10/29 01:10:40, mtomasz wrote: > nit: Shall we add jsdoc to these functions? I've always been of the opinion that tests should be small enough and well named enough that jsdoc is not needed. The big win in doing it this way is that when a test fails, the name alone provides a clear description of what the test was supposed to be guaranteeing...no need to lookup the comment in source. NOTE: Updated test method names to make it more explanatory. https://codereview.chromium.org/677213002/diff/160001/chrome/test/data/file_m... chrome/test/data/file_manager/unit_tests/import_history_unittest.js:156: On 2014/10/29 01:10:40, mtomasz wrote: > nit: \n\n -> \n Done. https://codereview.chromium.org/677213002/diff/160001/chrome/test/data/file_m... chrome/test/data/file_manager/unit_tests/import_history_unittest.js:158: function handleError_(callback, error) { On 2014/10/29 01:10:40, mtomasz wrote: > nit: Private in what scope? Maybe no need for _ at the end? > nit: jsdoc missing > nit: ; at the end redundant Hmmm. Good point. It would be file scoped if such a thing existed. Nuked @private. https://codereview.chromium.org/677213002/diff/160001/ui/file_manager/file_ma... File ui/file_manager/file_manager/background/js/import_history.js (right): https://codereview.chromium.org/677213002/diff/160001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/import_history.js:19: }; On 2014/10/29 01:10:41, mtomasz wrote: > nit: ; seems redundant here. Done. https://codereview.chromium.org/677213002/diff/160001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/import_history.js:160: RecordStorage.prototype.write; On 2014/10/29 01:10:41, mtomasz wrote: > Will this compile? We used to write an empty method implementation in > interfaces, like RS.prototype.write = function(record) {}. I'm fine with this, > as long as it compiles. This is purely in anticipation of Closure compilation. The @interface isn't used at all in uncompiled code (referenced only by @implements). It'll be used by the Closure Compiler during type checking && Closure Compiler will strip it from builds. https://codereview.chromium.org/677213002/diff/160001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/import_history.js:213: {type: 'application/json; charset=utf-8'}); On 2014/10/29 01:10:41, mtomasz wrote: > It's not really a valid json. How about simply plain/text mime type? Done. text/plain; charset=UTF-8 https://codereview.chromium.org/677213002/diff/160001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/import_history.js:230: .then(this.readFileAsText_.bind(this), function() { return '';}) On 2014/10/29 01:10:41, mtomasz wrote: > nit: space after ; missing. Done. https://codereview.chromium.org/677213002/diff/160001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/import_history.js:291: resolve(JSON.parse(json)); On 2014/10/29 01:10:41, mtomasz wrote: > Date objects will not deserialize properly from json. Shall we add a comment > about it, or add a special logic for creating Date objects when deserializing? The value is just a string. There's no need to parse the date. We just need a string for opaque comparison. The value used in this entry is provided by the fileEntry from the getMetadata call (lastModifiedTime or something like that). And since this is a first line of defence, best-effort type of operation, we don't really need to worry about a user who is switching locales. That would be a self-rectifying situation after a single import + the media would still be deduplicated at the content based deduplication phase.
lgtm
Basically looks good to me, but let me scan one more time. https://codereview.chromium.org/677213002/diff/180001/chrome/test/data/file_m... File chrome/test/data/file_manager/unit_tests/import_history_unittest.js (right): https://codereview.chromium.org/677213002/diff/180001/chrome/test/data/file_m... chrome/test/data/file_manager/unit_tests/import_history_unittest.js:7: var FILE_LASTMOD = new Date("Dec 4 1968").toString(); nit: Abbreviations are not allowed by our style guide. How about simply FILE_LAST_MODIFICATION? https://codereview.chromium.org/677213002/diff/180001/chrome/test/data/file_m... chrome/test/data/file_manager/unit_tests/import_history_unittest.js:168: * @param {function()} callback nit: Unexpected double space after @param? https://codereview.chromium.org/677213002/diff/180001/ui/file_manager/file_ma... File ui/file_manager/file_manager/background/js/import_history.js (right): https://codereview.chromium.org/677213002/diff/180001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/import_history.js:116: * @param {string} key @private seems missing. Please check other methods too. https://codereview.chromium.org/677213002/diff/180001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/import_history.js:180: }; nit: Redundant ; since no assignment. https://codereview.chromium.org/677213002/diff/180001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/import_history.js:303: * An Object storage mechanism built on top of a FileEntry. I'm not really convinced about the description. Is it really a store mechanism? It looks like a simple wrapper converting methods promise compatible ones. nit: How about renaming to PromisaryEntry so it's self explaining what is actually the purpose of this wrapper? https://codereview.chromium.org/677213002/diff/180001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/import_history.js:308: * @private nit: Is this class private? https://codereview.chromium.org/677213002/diff/180001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/import_history.js:333: * Either a string, or an object. It is an Object.
On 2014/10/30 01:04:55, mtomasz wrote: > Basically looks good to me, but let me scan one more time. > > https://codereview.chromium.org/677213002/diff/180001/chrome/test/data/file_m... > File chrome/test/data/file_manager/unit_tests/import_history_unittest.js > (right): > > https://codereview.chromium.org/677213002/diff/180001/chrome/test/data/file_m... > chrome/test/data/file_manager/unit_tests/import_history_unittest.js:7: var > FILE_LASTMOD = new Date("Dec 4 1968").toString(); > nit: Abbreviations are not allowed by our style guide. How about simply > FILE_LAST_MODIFICATION? > > https://codereview.chromium.org/677213002/diff/180001/chrome/test/data/file_m... > chrome/test/data/file_manager/unit_tests/import_history_unittest.js:168: * > @param {function()} callback > nit: Unexpected double space after @param? > > https://codereview.chromium.org/677213002/diff/180001/ui/file_manager/file_ma... > File ui/file_manager/file_manager/background/js/import_history.js (right): > > https://codereview.chromium.org/677213002/diff/180001/ui/file_manager/file_ma... > ui/file_manager/file_manager/background/js/import_history.js:116: * @param > {string} key > @private seems missing. Please check other methods too. > > https://codereview.chromium.org/677213002/diff/180001/ui/file_manager/file_ma... > ui/file_manager/file_manager/background/js/import_history.js:180: }; > nit: Redundant ; since no assignment. > > https://codereview.chromium.org/677213002/diff/180001/ui/file_manager/file_ma... > ui/file_manager/file_manager/background/js/import_history.js:303: * An Object > storage mechanism built on top of a FileEntry. > I'm not really convinced about the description. Is it really a store mechanism? > It looks like a simple wrapper converting methods promise compatible ones. > > nit: How about renaming to PromisaryEntry so it's self explaining what is > actually the purpose of this wrapper? > > https://codereview.chromium.org/677213002/diff/180001/ui/file_manager/file_ma... > ui/file_manager/file_manager/background/js/import_history.js:308: * @private > nit: Is this class private? > > https://codereview.chromium.org/677213002/diff/180001/ui/file_manager/file_ma... > ui/file_manager/file_manager/background/js/import_history.js:333: * Either a > string, or an object. > It is an Object. One more thing, we should be able to use Closure compile to catch most of the tests. @fukino knows how. I'll find out how quickly, give me a sec.
On 2014/10/30 01:04:55, mtomasz wrote: > Basically looks good to me, but let me scan one more time. > > https://codereview.chromium.org/677213002/diff/180001/chrome/test/data/file_m... > File chrome/test/data/file_manager/unit_tests/import_history_unittest.js > (right): > > https://codereview.chromium.org/677213002/diff/180001/chrome/test/data/file_m... > chrome/test/data/file_manager/unit_tests/import_history_unittest.js:7: var > FILE_LASTMOD = new Date("Dec 4 1968").toString(); > nit: Abbreviations are not allowed by our style guide. How about simply > FILE_LAST_MODIFICATION? > > https://codereview.chromium.org/677213002/diff/180001/chrome/test/data/file_m... > chrome/test/data/file_manager/unit_tests/import_history_unittest.js:168: * > @param {function()} callback > nit: Unexpected double space after @param? > > https://codereview.chromium.org/677213002/diff/180001/ui/file_manager/file_ma... > File ui/file_manager/file_manager/background/js/import_history.js (right): > > https://codereview.chromium.org/677213002/diff/180001/ui/file_manager/file_ma... > ui/file_manager/file_manager/background/js/import_history.js:116: * @param > {string} key > @private seems missing. Please check other methods too. > > https://codereview.chromium.org/677213002/diff/180001/ui/file_manager/file_ma... > ui/file_manager/file_manager/background/js/import_history.js:180: }; > nit: Redundant ; since no assignment. > > https://codereview.chromium.org/677213002/diff/180001/ui/file_manager/file_ma... > ui/file_manager/file_manager/background/js/import_history.js:303: * An Object > storage mechanism built on top of a FileEntry. > I'm not really convinced about the description. Is it really a store mechanism? > It looks like a simple wrapper converting methods promise compatible ones. > > nit: How about renaming to PromisaryEntry so it's self explaining what is > actually the purpose of this wrapper? > > https://codereview.chromium.org/677213002/diff/180001/ui/file_manager/file_ma... > ui/file_manager/file_manager/background/js/import_history.js:308: * @private > nit: Is this class private? > > https://codereview.chromium.org/677213002/diff/180001/ui/file_manager/file_ma... > ui/file_manager/file_manager/background/js/import_history.js:333: * Either a > string, or an object. > It is an Object. One more thing, we should be able to use Closure compile to catch most of the tests. @fukino knows how. I'll find out how quickly, give me a sec.
On 2014/10/30 01:27:10, mtomasz wrote: > On 2014/10/30 01:04:55, mtomasz wrote: > > Basically looks good to me, but let me scan one more time. > > > > > https://codereview.chromium.org/677213002/diff/180001/chrome/test/data/file_m... > > File chrome/test/data/file_manager/unit_tests/import_history_unittest.js > > (right): > > > > > https://codereview.chromium.org/677213002/diff/180001/chrome/test/data/file_m... > > chrome/test/data/file_manager/unit_tests/import_history_unittest.js:7: var > > FILE_LASTMOD = new Date("Dec 4 1968").toString(); > > nit: Abbreviations are not allowed by our style guide. How about simply > > FILE_LAST_MODIFICATION? > > > > > https://codereview.chromium.org/677213002/diff/180001/chrome/test/data/file_m... > > chrome/test/data/file_manager/unit_tests/import_history_unittest.js:168: * > > @param {function()} callback > > nit: Unexpected double space after @param? > > > > > https://codereview.chromium.org/677213002/diff/180001/ui/file_manager/file_ma... > > File ui/file_manager/file_manager/background/js/import_history.js (right): > > > > > https://codereview.chromium.org/677213002/diff/180001/ui/file_manager/file_ma... > > ui/file_manager/file_manager/background/js/import_history.js:116: * @param > > {string} key > > @private seems missing. Please check other methods too. > > > > > https://codereview.chromium.org/677213002/diff/180001/ui/file_manager/file_ma... > > ui/file_manager/file_manager/background/js/import_history.js:180: }; > > nit: Redundant ; since no assignment. > > > > > https://codereview.chromium.org/677213002/diff/180001/ui/file_manager/file_ma... > > ui/file_manager/file_manager/background/js/import_history.js:303: * An Object > > storage mechanism built on top of a FileEntry. > > I'm not really convinced about the description. Is it really a store > mechanism? > > It looks like a simple wrapper converting methods promise compatible ones. > > > > nit: How about renaming to PromisaryEntry so it's self explaining what is > > actually the purpose of this wrapper? > > > > > https://codereview.chromium.org/677213002/diff/180001/ui/file_manager/file_ma... > > ui/file_manager/file_manager/background/js/import_history.js:308: * @private > > nit: Is this class private? > > > > > https://codereview.chromium.org/677213002/diff/180001/ui/file_manager/file_ma... > > ui/file_manager/file_manager/background/js/import_history.js:333: * Either > a > > string, or an object. > > It is an Object. > > One more thing, we should be able to use Closure compile to catch most of the > tests. @fukino knows how. I'll find out how quickly, give me a sec. s/tests/bugs/.
PTALs. https://codereview.chromium.org/677213002/diff/180001/chrome/test/data/file_m... File chrome/test/data/file_manager/unit_tests/import_history_unittest.js (right): https://codereview.chromium.org/677213002/diff/180001/chrome/test/data/file_m... chrome/test/data/file_manager/unit_tests/import_history_unittest.js:7: var FILE_LASTMOD = new Date("Dec 4 1968").toString(); On 2014/10/30 01:04:54, mtomasz wrote: > nit: Abbreviations are not allowed by our style guide. How about simply > FILE_LAST_MODIFICATION? Done. https://codereview.chromium.org/677213002/diff/180001/chrome/test/data/file_m... chrome/test/data/file_manager/unit_tests/import_history_unittest.js:168: * @param {function()} callback On 2014/10/30 01:04:54, mtomasz wrote: > nit: Unexpected double space after @param? Done. https://codereview.chromium.org/677213002/diff/180001/ui/file_manager/file_ma... File ui/file_manager/file_manager/background/js/import_history.js (right): https://codereview.chromium.org/677213002/diff/180001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/import_history.js:116: * @param {string} key On 2014/10/30 01:04:55, mtomasz wrote: > @private seems missing. Please check other methods too. Done. https://codereview.chromium.org/677213002/diff/180001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/import_history.js:180: }; On 2014/10/30 01:04:55, mtomasz wrote: > nit: Redundant ; since no assignment. Done. https://codereview.chromium.org/677213002/diff/180001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/import_history.js:303: * An Object storage mechanism built on top of a FileEntry. On 2014/10/30 01:04:55, mtomasz wrote: > I'm not really convinced about the description. Is it really a store mechanism? > It looks like a simple wrapper converting methods promise compatible ones. > > nit: How about renaming to PromisaryEntry so it's self explaining what is > actually the purpose of this wrapper? Woops! That was just a wrong comment. Done. https://codereview.chromium.org/677213002/diff/180001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/import_history.js:308: * @private On 2014/10/30 01:04:55, mtomasz wrote: > nit: Is this class private? Done. https://codereview.chromium.org/677213002/diff/180001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/import_history.js:333: * Either a string, or an object. On 2014/10/30 01:04:54, mtomasz wrote: > It is an Object. Done.
lgtm with a nit! https://codereview.chromium.org/677213002/diff/200001/ui/file_manager/file_ma... File ui/file_manager/file_manager/background/js/import_history.js (right): https://codereview.chromium.org/677213002/diff/200001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/import_history.js:307: * An wrapper for FileEntry that provides Promises. nit: A wrapper
lgtm for jsdocs.
Fix a comment typo.
Thanks for the review! Ben and I are both happy to be up and running working with you guys in the Files.app codebase! https://codereview.chromium.org/677213002/diff/200001/ui/file_manager/file_ma... File ui/file_manager/file_manager/background/js/import_history.js (right): https://codereview.chromium.org/677213002/diff/200001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/import_history.js:307: * An wrapper for FileEntry that provides Promises. On 2014/10/30 01:55:46, mtomasz wrote: > nit: A wrapper Done.
The CQ bit was checked by smckay@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/677213002/220001
Message was sent while issue was closed.
Committed patchset #11 (id:220001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/2dc842c7481b6cc806bcd7b8cd027e496853149f Cr-Commit-Position: refs/heads/master@{#302116} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
