Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
313001:
Implement the Extension History API, v 0.1.... (Closed)
|
Created:
11 years, 2 months ago by brg Modified:
9 years, 7 months ago CC:
chromium-reviews_googlegroups.com, Erik does not do reviews, ben+cc_chromium.org, pam+watch_chromium.org, Paweł Hajdan Jr. Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionImplement the Extension History API, v 0.1.The first version is a weak wrapper around the HistoryServices object in Chrome.BUG=22952
TEST=browser_tests.exe --gtest_filer=ExtensionApiTest.History
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=30561
Patch Set 1 #Patch Set 2 : '' #Patch Set 3 : '' #Patch Set 4 : '' #Patch Set 5 : '' #
Total comments: 167
Patch Set 6 : '' #Patch Set 7 : '' #
Total comments: 6
Patch Set 8 : '' #Patch Set 9 : '' #Patch Set 10 : '' #Patch Set 11 : '' #
Total comments: 50
Patch Set 12 : '' #Patch Set 13 : '' #
Total comments: 8
Patch Set 14 : '' #Patch Set 15 : '' #Patch Set 16 : '' #Patch Set 17 : '' #Messages
Total messages: 20 (0 generated)
Hello Erik, If you do not have the time or desire to code review, could you please recommend someone who can. I modeled this CL after you implementation of the bookmarks API. At this time, there is a need for comments. Please disregard the events firing class ExtensionHistoryEventRouter and history_view.html, both need work which I will pick up on in the morning. The currently exposed API is modeld after the History Service, with a goal to make the proposed API useful and complete (useful requires the URLID, complete in that history.clear() can be implemented via history.deleteRange, etc). I am more than willing to modify the API in response to other opinions.
ExtensionHistoryEventRouter is complete, and the onVisited and onRemoved APIs are working.
+brettw to review history API usage http://codereview.chromium.org/313001/diff/1009/1011 File chrome/browser/extensions/extension_history_module.cc (right): http://codereview.chromium.org/313001/diff/1009/1011#newcode34 Line 34: static void AddItemNode(const history::URLRow& row, ListValue* list) { nit: don't need static for functions in anon namespace http://codereview.chromium.org/313001/diff/1009/1011#newcode34 Line 34: static void AddItemNode(const history::URLRow& row, ListValue* list) { AddHistoryNode http://codereview.chromium.org/313001/diff/1009/1011#newcode51 Line 51: static void AddVisitNode(const history::VisitRow& row, ListValue* list) { nit: remove static http://codereview.chromium.org/313001/diff/1009/1011#newcode62 Line 62: nit: remove newline http://codereview.chromium.org/313001/diff/1009/1011#newcode68 Line 68: } nit: remove {} http://codereview.chromium.org/313001/diff/1009/1011#newcode151 Line 151: // Bookmarks are not ready yet. We'll wait. fix comment http://codereview.chromium.org/313001/diff/1009/1011#newcode164 Line 164: int64* id) { nit: indent http://codereview.chromium.org/313001/diff/1009/1011#newcode186 Line 186: bool AsyncHistoryFunction::AsyncMethodCalled() { This is a bit odd since it always returns true. It's like you're trying to do a macro without a macro. I would just do the AddRef(); return true; at each callsite. I think it's more clear. Maybe it would be better if we just changed AsyncExtensionFunction to AddRef() in Run() and Release() in SendResponse() so we don't have to worry about doing this manually. http://codereview.chromium.org/313001/diff/1009/1011#newcode194 Line 194: NewRunnableMethod(this, &AsyncHistoryFunction::SendResponseToCallback)); why not just call SendResponse(true) directly? http://codereview.chromium.org/313001/diff/1009/1011#newcode198 Line 198: nit: remove newline http://codereview.chromium.org/313001/diff/1009/1011#newcode213 Line 213: hs->QueryURL(url, true, &cancelable_consumer_, add comment next to true http://codereview.chromium.org/313001/diff/1009/1011#newcode229 Line 229: AddVisitNode(*iterator, list); indent http://codereview.chromium.org/313001/diff/1009/1011#newcode245 Line 245: options.most_recent_visit_only = true; this isn't in the function description in the api json http://codereview.chromium.org/313001/diff/1009/1011#newcode247 Line 247: options.max_count = 100; neither is this http://codereview.chromium.org/313001/diff/1009/1011#newcode252 Line 252: &date)); indent http://codereview.chromium.org/313001/diff/1009/1011#newcode259 Line 259: options.end_time = base::Time::FromDoubleT(date / 1000); should there be some validation that this is a reasonable date? (many callsites) http://codereview.chromium.org/313001/diff/1009/1011#newcode265 Line 265: options.max_count = max_count; this should be capped http://codereview.chromium.org/313001/diff/1009/1011#newcode284 Line 284: AddItemNode(**iterator, list); indent http://codereview.chromium.org/313001/diff/1009/1011#newcode321 Line 321: hs->DeleteURL(url); does this remove all visits as well? http://codereview.chromium.org/313001/diff/1009/1013 File chrome/browser/extensions/extension_history_module.h (right): http://codereview.chromium.org/313001/diff/1009/1013#newcode5 Line 5: #ifndef CHROME_BROWSER_EXTENSIONS_EXTENSION_HISTORY_MODULE_H_ The new naming style for api files is to use "api" instead of "module" (e.g. extension_history_api.h). Please rename. http://codereview.chromium.org/313001/diff/1009/1013#newcode27 Line 27: void Init(Profile* profile); Rename this to "AddProfile" or "ObserveProfile". I wonder if we have to have a corresponding RemoveProfile? http://codereview.chromium.org/313001/diff/1009/1013#newcode59 Line 59: class HistoryFunction : public AsyncExtensionFunction, Comments above classes (this and below). http://codereview.chromium.org/313001/diff/1009/1010 File chrome/browser/extensions/extension_history_module_constants.h (right): http://codereview.chromium.org/313001/diff/1009/1010#newcode40 Line 40: extern const char kNoSearchError[]; I didn't see this used anywhere http://codereview.chromium.org/313001/diff/1009/1017 File chrome/common/extensions/api/extension_api.json (right): http://codereview.chromium.org/313001/diff/1009/1017#newcode1335 Line 1335: "lastVisit": {"type": "number", "optional": true, "description": "When this page was loaded, represented in milliseconds since the epoch (new Date(dateAdded))."}, We should be consistent in naming these "date in ms" fields. You have "lastVisit", "visitTime", "startDate". The only other place we do this is in BookmarkNode where we use names like "dateAdded". Since these aren't Date objects, I wonder if we should use something like "visitDateInMs". The other option would be to add a "date" type to the JSON schema that handles the millisecond formatting automatically. (@aa, @rafaelw: do either of you have an opinion here?) http://codereview.chromium.org/313001/diff/1009/1017#newcode1336 Line 1336: "totalVisitCount": {"type": "number", "optional": true, "description": "The number of times the user has navigated to this page."}, "total" seems redundant here http://codereview.chromium.org/313001/diff/1009/1017#newcode1336 Line 1336: "totalVisitCount": {"type": "number", "optional": true, "description": "The number of times the user has navigated to this page."}, type should be "integer" http://codereview.chromium.org/313001/diff/1009/1017#newcode1337 Line 1337: "totalTypedCount": {"type": "number", "optional": true, "description": "The number of times the user has navigated to this page by typing in the address."}, and here http://codereview.chromium.org/313001/diff/1009/1017#newcode1337 Line 1337: "totalTypedCount": {"type": "number", "optional": true, "description": "The number of times the user has navigated to this page by typing in the address."}, type should be integer http://codereview.chromium.org/313001/diff/1009/1017#newcode1349 Line 1349: "transition": {"type": "number", "description": "The transition type for this visit from its referrer."} The "description" here needs to list out the values for this enum. Type should be integer. It should have a min and max value for the enum range. http://codereview.chromium.org/313001/diff/1009/1017#newcode1358 Line 1358: "startDate": {"type": "number", "optional": true, "description": "Limit queries to results added to history after this date."}, Be more clear about what date you're talking about. Is this based on lastVisit? http://codereview.chromium.org/313001/diff/1009/1017#newcode1360 Line 1360: "number": {"type": "number", "optional": true, "description": "The maximum number of results to retreive."}, the key name should be more descriptive: "maxResults" the type should be integer. there should be an enforced max value (I assume we don't want to allow someone to ask for a million results). http://codereview.chromium.org/313001/diff/1009/1017#newcode1374 Line 1374: "search": {"type": "string", "description": "A free-text query."}, The description needs to be more descriptive. A free-text query over what? (the text contents of the pages? the titles? the urls?) Is it an "AND" of all of the words or an "OR"? Are any operators supported? (e.g. is ["foo bar"] different from [foo bar]?) http://codereview.chromium.org/313001/diff/1009/1017#newcode1377 Line 1377: "number": {"type": "number", "optional": true, "description": "The maximum number of results to retreive."} "maxResults", type integer, min/max enforced http://codereview.chromium.org/313001/diff/1009/1017#newcode1390 Line 1390: "name": "queryUrl", queryURL http://codereview.chromium.org/313001/diff/1009/1017#newcode1390 Line 1390: "name": "queryUrl", I have a bunch of problems with this function: - inconsistent naming ("query" vs "search" above) - why no date range? - not clear from the name that it's specifically about visits (why is it?) - if it doesn't need to be about visits, should it just be combined with search above? http://codereview.chromium.org/313001/diff/1009/1017#newcode1396 Line 1396: "type": "string" add a description and be explicit about whether match is partial/prefix or exact http://codereview.chromium.org/313001/diff/1009/1017#newcode1408 Line 1408: "name": "addUrl", addURL http://codereview.chromium.org/313001/diff/1009/1017#newcode1408 Line 1408: "name": "addUrl", What's the use case for this? I don't think we should have this in the API. http://codereview.chromium.org/313001/diff/1009/1017#newcode1410 Line 1410: "description": "Adds a Url t the history.", typo: "to" nit: Url -> URL (there are many instances of this that I won't highlight individually) "to the history database." http://codereview.chromium.org/313001/diff/1009/1017#newcode1420 Line 1420: "name": "deleteUrl", deleteURL http://codereview.chromium.org/313001/diff/1009/1017#newcode1422 Line 1422: "description": "Removes all occurances of the given Url from the history.", spelling: occurrences nit: "from the history database." http://codereview.chromium.org/313001/diff/1009/1017#newcode1422 Line 1422: "description": "Removes all occurances of the given Url from the history.", can you combine deleteURL and deleteRange into a single function? http://codereview.chromium.org/313001/diff/1009/1017#newcode1434 Line 1434: "description": "Removes all items within the specified date range from the history.", "the history database" http://codereview.chromium.org/313001/diff/1009/1017#newcode1454 Line 1454: "name": "onVisited", This is confusing naming. If it's onVisited, I would expect it to be a VisitItem, not a HistoryItem. Is this for visits or new history entries? http://codereview.chromium.org/313001/diff/1009/1017#newcode1462 Line 1462: "name": "onRemoved", This seems like it could get called with a lot of data. From our UI, we only allow deletion by date. Maybe rather than the URLs, we should just include the date that was deleted? http://codereview.chromium.org/313001/diff/1009/1017#newcode1464 Line 1464: "description": "Fired when a history item is removed.", Flesh out this description. http://codereview.chromium.org/313001/diff/1009/1023 File chrome/test/data/extensions/api_test/history/test.js (right): http://codereview.chromium.org/313001/diff/1009/1023#newcode20 Line 20: setTimeout("my_wait_callback()", 3000); remove my_wait_variable, you can just say setTimeout(callback, 3000); http://codereview.chromium.org/313001/diff/1009/1023#newcode20 Line 20: setTimeout("my_wait_callback()", 3000); also, shouldn't 2 seconds be enough to ensure that dates don't overlap? (every second in reduced running time is useful and since this is called a bunch, it will add up) http://codereview.chromium.org/313001/diff/1009/1023#newcode26 Line 26: itemVisitedSemaphore = true; remove unused variable http://codereview.chromium.org/313001/diff/1009/1023#newcode36 Line 36: start_date = new Date(); Use var ahead of local variable declarations to avoid putting these into the global scope. You do this throughout the file. http://codereview.chromium.org/313001/diff/1009/1023#newcode37 Line 37: start_date.setFullYear(1970, 1, 1); find and replace all of the hacker-style variables (start_date) in this file with camelCase (startDate). http://codereview.chromium.org/313001/diff/1009/1023#newcode40 Line 40: chrome.history.deleteRange(start_date.getTime(), "delete all" seems like a common enough need that perhaps it should be simplified in the API. http://codereview.chromium.org/313001/diff/1009/1023#newcode53 Line 53: function populateHistory(urls, callback) { I would rather than this actually did some navigations to generate the history data. (you can use the mock resolver and a local server to do this reliably) This way you can have real text to search on as well. http://codereview.chromium.org/313001/diff/1009/1023#newcode62 Line 62: clearHistory(pass(function() { This seems like an odd first test. You could explicitly populate and then verify that history is non-empty first. http://codereview.chromium.org/313001/diff/1009/1023#newcode70 Line 70: clearHistory(pass(function() { Why clearHistory at the beginning of each test? These tests are guaranteed to run in order, so you can count on built-up state from the previous tests. http://codereview.chromium.org/313001/diff/1009/1023#newcode73 Line 73: query.search = ''; so "" matches everything? that seems like bad behavior. if we want to have an explicit "all" query, I'd prefer null for the search param and that it be called out explicitly in the docs. In practice however, history is so large, that an all query is a bad idea. Also, this test should really be exercising what search can do. http://codereview.chromium.org/313001/diff/1009/1023#newcode84 Line 84: start_date = new Date(); you should artificially subtract a few seconds here just to be sure there's no weird rounding differences in time between the history db and JS time. http://codereview.chromium.org/313001/diff/1009/1023#newcode88 Line 88: populateHistory(new Array(picasa_url), pass(function() { I think you need to wait a second before the populate as well. http://codereview.chromium.org/313001/diff/1009/1023#newcode90 Line 90: query.search = ''; again, search for something relevant. http://codereview.chromium.org/313001/diff/1009/1023#newcode103 Line 103: function time_scoped_search2() { a comment explaining the difference between these two tests would be good http://codereview.chromium.org/313001/diff/1009/1023#newcode108 Line 108: populateHistory(new Array(picasa_url), pass(function() { wait before this again http://codereview.chromium.org/313001/diff/1009/1023#newcode157 Line 157: nit: remove extra newline http://codereview.chromium.org/313001/diff/1009/1023#newcode177 Line 177: start_date = new Date(); again, artificially decrement the time here http://codereview.chromium.org/313001/diff/1009/1023#newcode181 Line 181: populateHistory(new Array(picasa_url), pass(function() { again, wait before populate http://codereview.chromium.org/313001/diff/1009/1023#newcode205 Line 205: start_date = new Date(); same date comments here http://codereview.chromium.org/313001/diff/1009/1023#newcode205 Line 205: start_date = new Date(); startDate http://codereview.chromium.org/313001/diff/1009/1023#newcode207 Line 207: end_date = new Date(); endDate http://codereview.chromium.org/313001/diff/1009/1023#newcode209 Line 209: startDate = start_date.getTime(); startTime http://codereview.chromium.org/313001/diff/1009/1023#newcode210 Line 210: endDate = end_date.getTime(); endTime http://codereview.chromium.org/313001/diff/1009/1023#newcode259 Line 259: var yesterday = new Date(); "today" and "yesterday" are misleading names. endDate and startDate are fine. http://codereview.chromium.org/313001/diff/1009/1023#newcode261 Line 261: var end_time = today.getTime(); endTime http://codereview.chromium.org/313001/diff/1009/1023#newcode262 Line 262: var start_time = yesterday.getTime(); startTime http://codereview.chromium.org/313001/diff/1009/1019 File chrome/test/data/extensions/samples/history/history_toolstrip.html (right): http://codereview.chromium.org/313001/diff/1009/1019#newcode1 Line 1: <!-- toolstrips have been deprecated. The easiest way to replace this is with a browser action. Either use a popup or a page in a tab. http://codereview.chromium.org/313001/diff/1009/1020 File chrome/test/data/extensions/samples/history/history_view.html (right): http://codereview.chromium.org/313001/diff/1009/1020#newcode32 Line 32: // Insure that we have access to the history API. nit: Ensure http://codereview.chromium.org/313001/diff/1009/1020#newcode33 Line 33: if (!chrome.history) this shouldn't happen. did you ever witness this? http://codereview.chromium.org/313001/diff/1009/1020#newcode38 Line 38: var data_suffix = "_data"; dataSuffix (change this style throughout file) http://codereview.chromium.org/313001/diff/1009/1020#newcode43 Line 43: log.innerHTML = log.innerHTML + "<table> <tr> <td>" + name + "</td><td>" + data nit: 80 cols http://codereview.chromium.org/313001/diff/1009/1020#newcode73 Line 73: var toggleDataDisplay = function(event) { with this var style function declaration, you must end the function with a ; - fix throughout (or change to use standard function declaration style). http://codereview.chromium.org/313001/diff/1009/1020#newcode105 Line 105: var getTitleElement = function(text) { getTitleElement -> createTitleElement http://codereview.chromium.org/313001/diff/1009/1020#newcode124 Line 124: var getTableElement = function(innerHTML) { createTableElement http://codereview.chromium.org/313001/diff/1009/1020#newcode133 Line 133: var getHiddenDivElement = function(id) { get->create http://codereview.chromium.org/313001/diff/1009/1020#newcode153 Line 153: var getVisitTableElement = function(url) { get->create http://codereview.chromium.org/313001/diff/1009/1020#newcode186 Line 186: nit, remove newline
http://codereview.chromium.org/313001/diff/1009/1011 File chrome/browser/extensions/extension_history_module.cc (right): http://codereview.chromium.org/313001/diff/1009/1011#newcode150 Line 150: if (!service->BackendLoaded()) { I didn't totally understand the structure of your code, but I think this backend loaded stuff (and the loaded notification) are unnecessary. I assume you copied this from the bookmark service. It's only needed because history sends some information to the bookmarks service for importing data from older versions (when bookmarks were stored in history), and the bookmarks service needs to know whether or not it has been sent (most of time, there's nothing to import, so it needs to know when the backend is loaded to make sure it's not getting anything). Just calling normal history queries don't need to check this flag at all (or wait for the notification), since the history service will create the backend on demand, and you're running asynchronously. http://codereview.chromium.org/313001/diff/1009/1011#newcode314 Line 314: GURL url(WideToASCII(query_url)); WideToASCII is incorrect here since the page can specify any string, not necessarily a canonical URL. Instead, use GetAsString(std::string). This goes for all other GURL conversions. I suggest a helper function that fills a GURL given args so each function doesn't have to build one itself. http://codereview.chromium.org/313001/diff/1009/1011#newcode321 Line 321: hs->DeleteURL(url); On 2009/10/21 17:25:39, Erik Kay wrote: > does this remove all visits as well? Yes, this is fine. http://codereview.chromium.org/313001/diff/1009/1011#newcode333 Line 333: base::Time begin_time = base::Time::FromDoubleT(start_date / 1000); Can you factor out all of the time conversions into one function that returns a base::Time? http://codereview.chromium.org/313001/diff/1009/1013 File chrome/browser/extensions/extension_history_module.h (right): http://codereview.chromium.org/313001/diff/1009/1013#newcode83 Line 83: CancelableRequestConsumerT<int, 0> cancelable_consumer_; It doesn't look like you use the ClientData stuff in CancelableRequestConsumer. So you can just say CancelableRequestConsumer cancelable_consumer_;
This is a first set of responses to changes. I will respond with a list of things that were not simply "Done." shortly. http://codereview.chromium.org/313001/diff/1009/1011 File chrome/browser/extensions/extension_history_module.cc (right): http://codereview.chromium.org/313001/diff/1009/1011#newcode34 Line 34: static void AddItemNode(const history::URLRow& row, ListValue* list) { On 2009/10/21 17:25:39, Erik Kay wrote: > nit: don't need static for functions in anon namespace Done. http://codereview.chromium.org/313001/diff/1009/1011#newcode34 Line 34: static void AddItemNode(const history::URLRow& row, ListValue* list) { On 2009/10/21 17:25:39, Erik Kay wrote: > nit: don't need static for functions in anon namespace Done. http://codereview.chromium.org/313001/diff/1009/1011#newcode51 Line 51: static void AddVisitNode(const history::VisitRow& row, ListValue* list) { On 2009/10/21 17:25:39, Erik Kay wrote: > nit: remove static Done. http://codereview.chromium.org/313001/diff/1009/1011#newcode62 Line 62: On 2009/10/21 17:25:39, Erik Kay wrote: > nit: remove newline Done. http://codereview.chromium.org/313001/diff/1009/1011#newcode68 Line 68: } On 2009/10/21 17:25:39, Erik Kay wrote: > nit: remove {} Done. http://codereview.chromium.org/313001/diff/1009/1011#newcode150 Line 150: if (!service->BackendLoaded()) { Yes, as I mentioned to Erik I used the bookmarks api structure as a starting point. I had thought it simple safer to keep this code since the history service does have a notification on startup. However, I will remove it as per your recommendation. http://codereview.chromium.org/313001/diff/1009/1011#newcode151 Line 151: // Bookmarks are not ready yet. We'll wait. On 2009/10/21 17:25:39, Erik Kay wrote: > fix comment Done. http://codereview.chromium.org/313001/diff/1009/1011#newcode164 Line 164: int64* id) { On 2009/10/21 17:25:39, Erik Kay wrote: > nit: indent Done. http://codereview.chromium.org/313001/diff/1009/1011#newcode186 Line 186: bool AsyncHistoryFunction::AsyncMethodCalled() { On 2009/10/21 17:25:39, Erik Kay wrote: > This is a bit odd since it always returns true. It's like you're trying to do a > macro without a macro. I would just do the AddRef(); return true; at each > callsite. I think it's more clear. > > Maybe it would be better if we just changed AsyncExtensionFunction to AddRef() > in Run() and Release() in SendResponse() so we don't have to worry about doing > this manually. As for the AddRef, that's a good suggestion and I will make that change. For the Release, that needs to occur after the callback has completed for the cancellable_consumer semantics. Hence the PostTask. PS: SendResponse is not virtual. Which makes sense. I added a SendAsyncReponse which replaces AsyncMethodCompleted. http://codereview.chromium.org/313001/diff/1009/1011#newcode194 Line 194: NewRunnableMethod(this, &AsyncHistoryFunction::SendResponseToCallback)); On 2009/10/21 17:25:39, Erik Kay wrote: > why not just call SendResponse(true) directly? I didn't want more action in the HistoryService callback than necessary. http://codereview.chromium.org/313001/diff/1009/1011#newcode198 Line 198: On 2009/10/21 17:25:39, Erik Kay wrote: > nit: remove newline Done. http://codereview.chromium.org/313001/diff/1009/1011#newcode213 Line 213: hs->QueryURL(url, true, &cancelable_consumer_, On 2009/10/21 17:25:39, Erik Kay wrote: > add comment next to true Done. http://codereview.chromium.org/313001/diff/1009/1011#newcode229 Line 229: AddVisitNode(*iterator, list); On 2009/10/21 17:25:39, Erik Kay wrote: > indent Done. http://codereview.chromium.org/313001/diff/1009/1011#newcode245 Line 245: options.most_recent_visit_only = true; On 2009/10/21 17:25:39, Erik Kay wrote: > this isn't in the function description in the api json This is how we set the lastVisit value. Since it was not explicitly stated that we will either give every reference or not, I choose to be terse here and verbose in the queryUrl method. http://codereview.chromium.org/313001/diff/1009/1011#newcode247 Line 247: options.max_count = 100; On 2009/10/21 17:25:39, Erik Kay wrote: > neither is this Added information about defaults, they were marked as optional already however. http://codereview.chromium.org/313001/diff/1009/1011#newcode252 Line 252: &date)); On 2009/10/21 17:25:39, Erik Kay wrote: > indent Done. http://codereview.chromium.org/313001/diff/1009/1011#newcode259 Line 259: options.end_time = base::Time::FromDoubleT(date / 1000); On 2009/10/21 17:25:39, Erik Kay wrote: > should there be some validation that this is a reasonable date? (many callsites) I will make a helper method for this, but I expect FromDoubleT to avoid turning negative times into huge times. Also, I would rather not through a JS error when startDate > endDate instead of simple returning an empty search result. http://codereview.chromium.org/313001/diff/1009/1011#newcode265 Line 265: options.max_count = max_count; On 2009/10/21 17:25:39, Erik Kay wrote: > this should be capped What is the reasoning for this? The default is low, but if a user wants all history whats the harm? http://codereview.chromium.org/313001/diff/1009/1011#newcode284 Line 284: AddItemNode(**iterator, list); On 2009/10/21 17:25:39, Erik Kay wrote: > indent Done. http://codereview.chromium.org/313001/diff/1009/1011#newcode314 Line 314: GURL url(WideToASCII(query_url)); On 2009/10/21 22:14:47, brettw wrote: > WideToASCII is incorrect here since the page can specify any string, not > necessarily a canonical URL. > > Instead, use GetAsString(std::string). This goes for all other GURL conversions. > I suggest a helper function that fills a GURL given args so each function > doesn't have to build one itself. Done. http://codereview.chromium.org/313001/diff/1009/1011#newcode321 Line 321: hs->DeleteURL(url); On 2009/10/21 17:25:39, Erik Kay wrote: > does this remove all visits as well? Yes, the API description in the JSON file mentions this. http://codereview.chromium.org/313001/diff/1009/1011#newcode321 Line 321: hs->DeleteURL(url); On 2009/10/21 22:14:47, brettw wrote: > On 2009/10/21 17:25:39, Erik Kay wrote: > > does this remove all visits as well? > > Yes, this is fine. Done. http://codereview.chromium.org/313001/diff/1009/1011#newcode333 Line 333: base::Time begin_time = base::Time::FromDoubleT(start_date / 1000); On 2009/10/21 22:14:47, brettw wrote: > Can you factor out all of the time conversions into one function that returns a > base::Time? Done. http://codereview.chromium.org/313001/diff/1009/1013 File chrome/browser/extensions/extension_history_module.h (right): http://codereview.chromium.org/313001/diff/1009/1013#newcode5 Line 5: #ifndef CHROME_BROWSER_EXTENSIONS_EXTENSION_HISTORY_MODULE_H_ On 2009/10/21 17:25:39, Erik Kay wrote: > The new naming style for api files is to use "api" instead of "module" (e.g. > extension_history_api.h). Please rename. Done. To preserve comments I will upload the changes asked for these files first, and then follow up with an upload for file renaming. http://codereview.chromium.org/313001/diff/1009/1013#newcode27 Line 27: void Init(Profile* profile); On 2009/10/21 17:25:39, Erik Kay wrote: > Rename this to "AddProfile" or "ObserveProfile". I wonder if we have to have a > corresponding RemoveProfile? I think we do not need a remove profile, in that we us the profile as provided in the Observe callback. The lookup table is to preserve const correctness. http://codereview.chromium.org/313001/diff/1009/1013#newcode59 Line 59: class HistoryFunction : public AsyncExtensionFunction, On 2009/10/21 17:25:39, Erik Kay wrote: > Comments above classes (this and below). Done. http://codereview.chromium.org/313001/diff/1009/1013#newcode83 Line 83: CancelableRequestConsumerT<int, 0> cancelable_consumer_; On 2009/10/21 22:14:47, brettw wrote: > It doesn't look like you use the ClientData stuff in CancelableRequestConsumer. > So you can just say > CancelableRequestConsumer cancelable_consumer_; Done. http://codereview.chromium.org/313001/diff/1009/1010 File chrome/browser/extensions/extension_history_module_constants.h (right): http://codereview.chromium.org/313001/diff/1009/1010#newcode40 Line 40: extern const char kNoSearchError[]; On 2009/10/21 17:25:39, Erik Kay wrote: > I didn't see this used anywhere Done. http://codereview.chromium.org/313001/diff/1009/1017 File chrome/common/extensions/api/extension_api.json (right): http://codereview.chromium.org/313001/diff/1009/1017#newcode1335 Line 1335: "lastVisit": {"type": "number", "optional": true, "description": "When this page was loaded, represented in milliseconds since the epoch (new Date(dateAdded))."}, On 2009/10/21 17:25:39, Erik Kay wrote: > We should be consistent in naming these "date in ms" fields. You have > "lastVisit", "visitTime", "startDate". The only other place we do this is in > BookmarkNode where we use names like "dateAdded". Since these aren't Date > objects, I wonder if we should use something like "visitDateInMs". The other > option would be to add a "date" type to the JSON schema that handles the > millisecond formatting automatically. (@aa, @rafaelw: do either of you have an > opinion here?) Changed all to xxxTime. http://codereview.chromium.org/313001/diff/1009/1017#newcode1336 Line 1336: "totalVisitCount": {"type": "number", "optional": true, "description": "The number of times the user has navigated to this page."}, On 2009/10/21 17:25:39, Erik Kay wrote: > "total" seems redundant here Done. http://codereview.chromium.org/313001/diff/1009/1017#newcode1336 Line 1336: "totalVisitCount": {"type": "number", "optional": true, "description": "The number of times the user has navigated to this page."}, On 2009/10/21 17:25:39, Erik Kay wrote: > type should be "integer" Done. http://codereview.chromium.org/313001/diff/1009/1017#newcode1337 Line 1337: "totalTypedCount": {"type": "number", "optional": true, "description": "The number of times the user has navigated to this page by typing in the address."}, On 2009/10/21 17:25:39, Erik Kay wrote: > and here Done. http://codereview.chromium.org/313001/diff/1009/1017#newcode1337 Line 1337: "totalTypedCount": {"type": "number", "optional": true, "description": "The number of times the user has navigated to this page by typing in the address."}, On 2009/10/21 17:25:39, Erik Kay wrote: > type should be integer Done. http://codereview.chromium.org/313001/diff/1009/1017#newcode1349 Line 1349: "transition": {"type": "number", "description": "The transition type for this visit from its referrer."} On 2009/10/21 17:25:39, Erik Kay wrote: > The "description" here needs to list out the values for this enum. > Type should be integer. It should have a min and max value for the enum range. Done. http://codereview.chromium.org/313001/diff/1009/1017#newcode1358 Line 1358: "startDate": {"type": "number", "optional": true, "description": "Limit queries to results added to history after this date."}, On 2009/10/21 17:25:39, Erik Kay wrote: > Be more clear about what date you're talking about. Is this based on lastVisit? Done. http://codereview.chromium.org/313001/diff/1009/1017#newcode1360 Line 1360: "number": {"type": "number", "optional": true, "description": "The maximum number of results to retreive."}, On 2009/10/21 17:25:39, Erik Kay wrote: > the key name should be more descriptive: "maxResults" > the type should be integer. there should be an enforced max value (I assume we > don't want to allow someone to ask for a million results). Again, I do not see the reason to limit an extension developer to a subset of history results. The default is 100, anything above that must be explicitly asked for. http://codereview.chromium.org/313001/diff/1009/1017#newcode1374 Line 1374: "search": {"type": "string", "description": "A free-text query."}, On 2009/10/21 17:25:39, Erik Kay wrote: > The description needs to be more descriptive. > A free-text query over what? (the text contents of the pages? the titles? the > urls?) Since this uses an underlying service, there is no gaurantee that any description here will remain correct. > Is it an "AND" of all of the words or an "OR"? Are any operators supported? > (e.g. is ["foo bar"] different from [foo bar]?) http://codereview.chromium.org/313001/diff/1009/1017#newcode1377 Line 1377: "number": {"type": "number", "optional": true, "description": "The maximum number of results to retreive."} On 2009/10/21 17:25:39, Erik Kay wrote: > "maxResults", type integer, min/max enforced Done as much as above. http://codereview.chromium.org/313001/diff/1009/1017#newcode1390 Line 1390: "name": "queryUrl", On 2009/10/21 17:25:39, Erik Kay wrote: > queryURL I prefer not to capitalize all letters of an acronym is it is more readable, e.g. XmlHttpRequest instead of XMLHTTPRequest. If this is non-standard or you strongly recommend changing, then I will. http://codereview.chromium.org/313001/diff/1009/1017#newcode1390 Line 1390: "name": "queryUrl", On 2009/10/21 17:25:39, Erik Kay wrote: > I have a bunch of problems with this function: > - inconsistent naming ("query" vs "search" above) > - why no date range? > - not clear from the name that it's specifically about visits (why is it?) > - if it doesn't need to be about visits, should it just be combined with search > above? I have changed it from queryUrl to urlVistits. There is a logical separation between the meta-data for items stored and the number of times those items have been acccessed, and that is reflected in the internal architecture. Now is that good for the API. It provides a simple means to build a table (key off the HistoryItem.id == VisitItem.id). I can get information for a single URL from one API, and information about last visits is not polluted with multiple browsing actions. http://codereview.chromium.org/313001/diff/1009/1017#newcode1396 Line 1396: "type": "string" On 2009/10/21 17:25:39, Erik Kay wrote: > add a description and be explicit about whether match is partial/prefix or exact Done. http://codereview.chromium.org/313001/diff/1009/1017#newcode1408 Line 1408: "name": "addUrl", On 2009/10/21 17:25:39, Erik Kay wrote: > addURL See above. http://codereview.chromium.org/313001/diff/1009/1017#newcode1408 Line 1408: "name": "addUrl", On 2009/10/21 17:25:39, Erik Kay wrote: > What's the use case for this? I don't think we should have this in the API. It was in the approved API, so I kept it. The primary use case I had was testing, although I can see someone using history as a database. http://codereview.chromium.org/313001/diff/1009/1017#newcode1410 Line 1410: "description": "Adds a Url t the history.", On 2009/10/21 17:25:39, Erik Kay wrote: > typo: "to" > nit: Url -> URL (there are many instances of this that I won't highlight > individually) > "to the history database." As above. http://codereview.chromium.org/313001/diff/1009/1017#newcode1420 Line 1420: "name": "deleteUrl", On 2009/10/21 17:25:39, Erik Kay wrote: > deleteURL As above. http://codereview.chromium.org/313001/diff/1009/1017#newcode1422 Line 1422: "description": "Removes all occurances of the given Url from the history.", On 2009/10/21 17:25:39, Erik Kay wrote: > spelling: occurrences > nit: "from the history database." Done. http://codereview.chromium.org/313001/diff/1009/1017#newcode1422 Line 1422: "description": "Removes all occurances of the given Url from the history.", On 2009/10/21 17:25:39, Erik Kay wrote: > spelling: occurrences > nit: "from the history database." Done. http://codereview.chromium.org/313001/diff/1009/1017#newcode1434 Line 1434: "description": "Removes all items within the specified date range from the history.", On 2009/10/21 17:25:39, Erik Kay wrote: > "the history database" Done. http://codereview.chromium.org/313001/diff/1009/1017#newcode1454 Line 1454: "name": "onVisited", On 2009/10/21 17:25:39, Erik Kay wrote: > This is confusing naming. If it's onVisited, I would expect it to be a > VisitItem, not a HistoryItem. Is this for visits or new history entries? New items are broadcast on visits, because one wants the URL and Title not the referal etc. Do you want a visit and an item information together? http://codereview.chromium.org/313001/diff/1009/1017#newcode1462 Line 1462: "name": "onRemoved", On 2009/10/21 17:25:39, Erik Kay wrote: > This seems like it could get called with a lot of data. From our UI, we only > allow deletion by date. Maybe rather than the URLs, we should just include the > date that was deleted? The UI is terrible, as we have only "delete this day." Have you ever tried to manually remove a week or specific entries? When history removes a URL it removes the entire visit information. To remove a single occurance of a visit, one uses the deleteRange API provided. Url information is gone by the time this API is called and further information can not be retreived. http://codereview.chromium.org/313001/diff/1009/1017#newcode1464 Line 1464: "description": "Fired when a history item is removed.", On 2009/10/21 17:25:39, Erik Kay wrote: > Flesh out this description. Done. http://codereview.chromium.org/313001/diff/1009/1023 File chrome/test/data/extensions/api_test/history/test.js (right): http://codereview.chromium.org/313001/diff/1009/1023#newcode20 Line 20: setTimeout("my_wait_callback()", 3000); On 2009/10/21 17:25:39, Erik Kay wrote: > also, shouldn't 2 seconds be enough to ensure that dates don't overlap? (every > second in reduced running time is useful and since this is called a bunch, it > will add up) Done. http://codereview.chromium.org/313001/diff/1009/1023#newcode20 Line 20: setTimeout("my_wait_callback()", 3000); On 2009/10/21 17:25:39, Erik Kay wrote: > remove my_wait_variable, you can just say setTimeout(callback, 3000); > Done. http://codereview.chromium.org/313001/diff/1009/1023#newcode26 Line 26: itemVisitedSemaphore = true; On 2009/10/21 17:25:39, Erik Kay wrote: > remove unused variable Done. http://codereview.chromium.org/313001/diff/1009/1023#newcode36 Line 36: start_date = new Date(); On 2009/10/21 17:25:39, Erik Kay wrote: > Use var ahead of local variable declarations to avoid putting these into the > global scope. You do this throughout the file. Done. http://codereview.chromium.org/313001/diff/1009/1023#newcode37 Line 37: start_date.setFullYear(1970, 1, 1); On 2009/10/21 17:25:39, Erik Kay wrote: > find and replace all of the hacker-style variables (start_date) in this file > with camelCase (startDate). Done. For me, if you give general comments like this I will search through the files for similar cases. You need not attempt to call out every instance if you want to save that time, but I do thank you for your thoroughness. http://codereview.chromium.org/313001/diff/1009/1023#newcode40 Line 40: chrome.history.deleteRange(start_date.getTime(), On 2009/10/21 17:25:39, Erik Kay wrote: > "delete all" seems like a common enough need that perhaps it should be > simplified in the API. It would be nice to extend the API through JS instead of having to expose more JS/C++ bindings. http://codereview.chromium.org/313001/diff/1009/1023#newcode53 Line 53: function populateHistory(urls, callback) { On 2009/10/21 17:25:39, Erik Kay wrote: > I would rather than this actually did some navigations to generate the history > data. (you can use the mock resolver and a local server to do this reliably) > This way you can have real text to search on as well. I will add tests that do this for text query. http://codereview.chromium.org/313001/diff/1009/1023#newcode62 Line 62: clearHistory(pass(function() { On 2009/10/21 17:25:39, Erik Kay wrote: > This seems like an odd first test. You could explicitly populate and then > verify that history is non-empty first. All the tests require a blank starting state. This test is insuring that we can set that state. I would rather keep this test and document it. http://codereview.chromium.org/313001/diff/1009/1023#newcode70 Line 70: clearHistory(pass(function() { On 2009/10/21 17:25:39, Erik Kay wrote: > Why clearHistory at the beginning of each test? These tests are guaranteed to > run in order, so you can count on built-up state from the previous tests. Relying on the side-effects of previous tests is not going to make them maintainable. I would rather keep each test self-contained. http://codereview.chromium.org/313001/diff/1009/1023#newcode73 Line 73: query.search = ''; On 2009/10/21 17:25:39, Erik Kay wrote: > so "" matches everything? that seems like bad behavior. if we want to have an > explicit "all" query, I'd prefer null for the search param and that it be called > out explicitly in the docs. In practice however, history is so large, that an > all query is a bad idea. > > Also, this test should really be exercising what search can do. As mentioned, I will add some navigates+search. The "" search is how the UI works, and is the only means for an extension developer to query all history. http://codereview.chromium.org/313001/diff/1009/1023#newcode84 Line 84: start_date = new Date(); On 2009/10/21 17:25:39, Erik Kay wrote: > you should artificially subtract a few seconds here just to be sure there's no > weird rounding differences in time between the history db and JS time. Done here and everywhere. http://codereview.chromium.org/313001/diff/1009/1023#newcode88 Line 88: populateHistory(new Array(picasa_url), pass(function() { On 2009/10/21 17:25:39, Erik Kay wrote: > I think you need to wait a second before the populate as well. the wait is called above this. the add is flushed from the service's working thread by the query call which follows. http://codereview.chromium.org/313001/diff/1009/1023#newcode90 Line 90: query.search = ''; On 2009/10/21 17:25:39, Erik Kay wrote: > again, search for something relevant. What I want is to get the full state and verify it (number of elements, what those elements are). I will add tests for text search. http://codereview.chromium.org/313001/diff/1009/1023#newcode103 Line 103: function time_scoped_search2() { On 2009/10/21 17:25:39, Erik Kay wrote: > a comment explaining the difference between these two tests would be good Done. http://codereview.chromium.org/313001/diff/1009/1023#newcode108 Line 108: populateHistory(new Array(picasa_url), pass(function() { On 2009/10/21 17:25:39, Erik Kay wrote: > wait before this again Could you clarify as to why? The wait needed to occur between the two calls to populate. http://codereview.chromium.org/313001/diff/1009/1023#newcode157 Line 157: On 2009/10/21 17:25:39, Erik Kay wrote: > nit: remove extra newline Done. http://codereview.chromium.org/313001/diff/1009/1023#newcode177 Line 177: start_date = new Date(); On 2009/10/21 17:25:39, Erik Kay wrote: > again, artificially decrement the time here Done. http://codereview.chromium.org/313001/diff/1009/1023#newcode181 Line 181: populateHistory(new Array(picasa_url), pass(function() { On 2009/10/21 17:25:39, Erik Kay wrote: > again, wait before populate See above. http://codereview.chromium.org/313001/diff/1009/1023#newcode205 Line 205: start_date = new Date(); On 2009/10/21 17:25:39, Erik Kay wrote: > same date comments here Done. http://codereview.chromium.org/313001/diff/1009/1023#newcode205 Line 205: start_date = new Date(); On 2009/10/21 17:25:39, Erik Kay wrote: > same date comments here Done. http://codereview.chromium.org/313001/diff/1009/1023#newcode207 Line 207: end_date = new Date(); On 2009/10/21 17:25:39, Erik Kay wrote: > endDate Done. http://codereview.chromium.org/313001/diff/1009/1023#newcode209 Line 209: startDate = start_date.getTime(); On 2009/10/21 17:25:39, Erik Kay wrote: > startTime removed this completely. http://codereview.chromium.org/313001/diff/1009/1023#newcode210 Line 210: endDate = end_date.getTime(); On 2009/10/21 17:25:39, Erik Kay wrote: > endTime removed this completely. http://codereview.chromium.org/313001/diff/1009/1023#newcode259 Line 259: var yesterday = new Date(); On 2009/10/21 17:25:39, Erik Kay wrote: > "today" and "yesterday" are misleading names. endDate and startDate are fine. Done. http://codereview.chromium.org/313001/diff/1009/1023#newcode261 Line 261: var end_time = today.getTime(); On 2009/10/21 17:25:39, Erik Kay wrote: > endTime Done. http://codereview.chromium.org/313001/diff/1009/1023#newcode262 Line 262: var start_time = yesterday.getTime(); On 2009/10/21 17:25:39, Erik Kay wrote: > startTime Done. http://codereview.chromium.org/313001/diff/1009/1019 File chrome/test/data/extensions/samples/history/history_toolstrip.html (right): http://codereview.chromium.org/313001/diff/1009/1019#newcode1 Line 1: <!-- On 2009/10/21 17:25:39, Erik Kay wrote: > toolstrips have been deprecated. The easiest way to replace this is with a > browser action. Either use a popup or a page in a tab. Would it be easiest to simply remove this, as it is hacky test code anyway?
Cleanup of auto-formatting failures and some missed issues. The next publish will be the file renaming, and it should be ready for review part 2. http://codereview.chromium.org/313001/diff/1009/1017 File chrome/common/extensions/api/extension_api.json (right): http://codereview.chromium.org/313001/diff/1009/1017#newcode1454 Line 1454: "name": "onVisited", On 2009/10/22 02:13:34, brg wrote: > On 2009/10/21 17:25:39, Erik Kay wrote: > > This is confusing naming. If it's onVisited, I would expect it to be a > > VisitItem, not a HistoryItem. Is this for visits or new history entries? > > New items are broadcast on visits, because one wants the URL and Title not the > referal etc. Do you want a visit and an item information together? Also I should point out that there is no distinction between a creation and a visits from the observer's view. One can be made by doing a subsequent query for visits, but that is adding another async call behind this api. http://codereview.chromium.org/313001/diff/6008/5014 File chrome/browser/extensions/extension_history_module.cc (right): http://codereview.chromium.org/313001/diff/6008/5014#newcode24 Line 24: double MillisecondsFromTime(const base::Time& time) { Camel case is wrong. Will be fixed in next upload. http://codereview.chromium.org/313001/diff/6008/5014#newcode226 Line 226: AddVisitNode(*iterator, list); Incorrect indentation. http://codereview.chromium.org/313001/diff/6008/5023 File chrome/test/data/extensions/api_test/history/test.js (right): http://codereview.chromium.org/313001/diff/6008/5023#newcode276 Line 276: chrome.history.deleteRange(startDate.getTime(), Something caused and auto-format through the IDE. This will be fixed in the next upload.
The file names are now updated to reflect the current standard. Please take a second look. Most of the issues raised by Erik's review were addressed. The four(?) main ones that were not completely addressed were, in order of least importance: 1) capitalization of acronyms in method names (deleteURL vs. deleteUrl). I prefer the latter. 2) Limiting search to xxx number of results. I believe that setting the default value to 100 and making query.maxResults optional goes a long way in keeping the number of results down. Moreover, since results are unique by URL, there is little chance of someone running out of memory from a search. There will not be 1mil search results unless someone is mapping all of the web by automation. 3) startDate, lastAccess, etc. All such things have been changed to startTime, etc. There was no word from aa on what was standard. 4) queryUrl was changed to urlVisits. I may have addressed your concerns in the comment, but if not please bring up the issue again. http://codereview.chromium.org/313001/diff/6008/5014 File chrome/browser/extensions/extension_history_module.cc (right): http://codereview.chromium.org/313001/diff/6008/5014#newcode24 Line 24: double MillisecondsFromTime(const base::Time& time) { On 2009/10/22 06:53:37, brg wrote: > Camel case is wrong. Will be fixed in next upload. Done. http://codereview.chromium.org/313001/diff/6008/5014#newcode226 Line 226: AddVisitNode(*iterator, list); On 2009/10/22 06:53:37, brg wrote: > Incorrect indentation. Done. http://codereview.chromium.org/313001/diff/6008/5023 File chrome/test/data/extensions/api_test/history/test.js (right): http://codereview.chromium.org/313001/diff/6008/5023#newcode276 Line 276: chrome.history.deleteRange(startDate.getTime(), On 2009/10/22 06:53:37, brg wrote: > Something caused and auto-format through the IDE. This will be fixed in the > next upload. Done.
I don't have any more comments on history API usage.
Patch set 10 makes changes to test.js. The changes are +JS readability (semi-colons, comments, object construction) +remove hacky wait loop from full_test_search and make it event driven.
Updated to include aa as a reviewer since Erik is on vacation.
This is really nice work. http://codereview.chromium.org/313001/diff/18001/18016 File chrome/browser/extensions/extension_history_api.h (right): http://codereview.chromium.org/313001/diff/18001/18016#newcode21 Line 21: virtual ~ExtensionHistoryEventRouter() {} This seems unnecessary unless you have someone inheriting this class. http://codereview.chromium.org/313001/diff/18001/18016#newcode21 Line 21: virtual ~ExtensionHistoryEventRouter() {} Also, can you get away with this being private? (you don't really want someone deleting your singleton). http://codereview.chromium.org/313001/diff/18001/18016#newcode23 Line 23: // Single instance of the observer. Nit: s/observer/router. http://codereview.chromium.org/313001/diff/18001/18016#newcode30 Line 30: virtual void Observe(NotificationType type, Looks like this can be private. http://codereview.chromium.org/313001/diff/18001/18016#newcode35 Line 35: typedef std::map<uintptr_t, Profile*> ProfileMap; Even though the styleguide says to put typedefs near the top of the block, we usually define these helper typdefs for particular collections right above their member. So in this case, right above profiles_. http://codereview.chromium.org/313001/diff/18001/18016#newcode72 Line 72: class AsyncHistoryFunction : public HistoryFunction { It is weird to have this function called AsyncHistoryFunction when you already have HistoryFunction which is deriving from AsyncFunction. It looks like in the two cases where you are using HistoryFunction they are indeed synchronous cases, so it would be better to inherit from SyncExtensionFunction not AsyncExtensionFunction. If you need to share code between these two base classes you could do it with helper functions. http://codereview.chromium.org/313001/diff/18001/18014 File chrome/browser/extensions/extension_history_api_constants.cc (right): http://codereview.chromium.org/313001/diff/18001/18014#newcode9 Line 9: const wchar_t kAllHistoryKey[] = L"all_history"; allHistory http://codereview.chromium.org/313001/diff/18001/18005 File chrome/common/extensions/api/extension_api.json (right): http://codereview.chromium.org/313001/diff/18001/18005#newcode1334 Line 1334: "title": {"type": "string", "optional": true, "description": "The text displayed for the node."}, "The title of the history page"? http://codereview.chromium.org/313001/diff/18001/18005#newcode1335 Line 1335: "lastVisitTime": {"type": "number", "optional": true, "description": "When this page was last loaded, represented in milliseconds since the epoch (new Date(dateAdded))."}, You can remove the parenthetical at the end of this. http://codereview.chromium.org/313001/diff/18001/18005#newcode1349 Line 1349: "transition": {"type": "integer", "minimum": 0, "maximum": 10, "description": "The transition type for this visit from its referrer. The enumeration is: LINK=0, TYPED = 1, AUTO_BOOKMARK = 2, AUTO_SUBFRAME = 3, MANUAL_SUBFRAME = 4, GENERATED = 5, START_PAGE = 6, FORM_SUBMIT = 7, RELOAD = 8, KEYWORD = 9, KEYWORD_GENERATED = 10"} It would be cool to setup an actual enum for these values. You can do it manually in extension_process_bindings.js, like: chrome.history.transitionType = { LINK: 0, ... } http://codereview.chromium.org/313001/diff/18001/18005#newcode1379 Line 1379: "name": "urlVisits", Method names should be verbs. How about getVisits()? http://codereview.chromium.org/313001/diff/18001/18005#newcode1402 Line 1402: "name": "url", For forward flexibility, the parameter should be an object with one property, 'url'. Also, I think it would make more sense to name this 'addVisit()'. http://codereview.chromium.org/313001/diff/18001/18005#newcode1414 Line 1414: "name": "url", Again with an object with a single param here. http://codereview.chromium.org/313001/diff/18001/18005#newcode1425 Line 1425: { The signature here should be a single object with multiple properties. http://codereview.chromium.org/313001/diff/18001/18005#newcode1441 Line 1441: "name": "clear", For consistency with the above, how about calling this 'deleteAll()'? http://codereview.chromium.org/313001/diff/18001/18005#newcode1461 Line 1461: "name": "onRemoved", For consistency, I think this should be named 'onDeleted'. http://codereview.chromium.org/313001/diff/18001/18005#newcode1469 Line 1469: "all_history": { "type": "boolean", "description": "True if all history was removed." }, camelCase. http://codereview.chromium.org/313001/diff/18001/18005#newcode1470 Line 1470: "urls": { "type": "array", "items": { "type": "string" }, "optional": true} Couldn't this be a lot if a range was removed? http://codereview.chromium.org/313001/diff/18001/18008 File chrome/test/data/extensions/api_test/history/test.js (right): http://codereview.chromium.org/313001/diff/18001/18008#newcode104 Line 104: function basic_search() { camelCase in JavaScript http://codereview.chromium.org/313001/diff/18001/18008#newcode107 Line 107: waitAFewSeconds(0.5, pass(function() { Why are the timeouts required. These need to be removed; they will result in flaky tests. http://codereview.chromium.org/313001/diff/18001/18008#newcode108 Line 108: var query = new Object; Nit: would be more javascripty to say: var query = { search: '' };
http://codereview.chromium.org/313001/diff/18001/18013 File chrome/browser/extensions/extension_history_api.cc (right): http://codereview.chromium.org/313001/diff/18001/18013#newcode190 Line 190: MessageLoop::current()->PostTask( Why post a message to the current message loop here, instead of just calling SendResponse() immediately?
Updated. The major change is that all tests are now event driven. Minor changes are naming updates. http://codereview.chromium.org/313001/diff/18001/18013 File chrome/browser/extensions/extension_history_api.cc (right): http://codereview.chromium.org/313001/diff/18001/18013#newcode190 Line 190: MessageLoop::current()->PostTask( On 2009/10/28 08:10:08, Aaron Boodman wrote: > Why post a message to the current message loop here, instead of just calling > SendResponse() immediately? Since I AddRef(), I must Release(). I can not Release() here, due to the semantics of calling the history thread with a CancellableConsumer (which must be acknowledged after the callback returns). So I must at least PostTask to Release(). But since I am PostTask'ing, I may as well be a good callback citizen and call SendResponse from there in case the callback loop ever becomes serialized and synchronous. http://codereview.chromium.org/313001/diff/18001/18016 File chrome/browser/extensions/extension_history_api.h (right): http://codereview.chromium.org/313001/diff/18001/18016#newcode21 Line 21: virtual ~ExtensionHistoryEventRouter() {} On 2009/10/28 07:06:00, Aaron Boodman wrote: > This seems unnecessary unless you have someone inheriting this class. Done. http://codereview.chromium.org/313001/diff/18001/18016#newcode21 Line 21: virtual ~ExtensionHistoryEventRouter() {} On 2009/10/28 07:06:00, Aaron Boodman wrote: > Also, can you get away with this being private? (you don't really want someone > deleting your singleton). Done. http://codereview.chromium.org/313001/diff/18001/18016#newcode23 Line 23: // Single instance of the observer. On 2009/10/28 07:06:00, Aaron Boodman wrote: > Nit: s/observer/router. Done. http://codereview.chromium.org/313001/diff/18001/18016#newcode30 Line 30: virtual void Observe(NotificationType type, On 2009/10/28 07:06:00, Aaron Boodman wrote: > Looks like this can be private. Done. http://codereview.chromium.org/313001/diff/18001/18016#newcode35 Line 35: typedef std::map<uintptr_t, Profile*> ProfileMap; On 2009/10/28 07:06:00, Aaron Boodman wrote: > Even though the styleguide says to put typedefs near the top of the block, we > usually define these helper typdefs for particular collections right above their > member. So in this case, right above profiles_. Done. http://codereview.chromium.org/313001/diff/18001/18016#newcode72 Line 72: class AsyncHistoryFunction : public HistoryFunction { In the comment for the class, AsyncExtensionFunction is named as such because it is asynchronous with respect to the browser thread. AsyncHistoryFunctions are async with respect to their caller. So the term is being overloaded. Perhaps HistoryFunctionWithCallback? But since addUrl and deleteUrl are indeed async to the browser thread should they really be changed? They use the helper function GetUrlFromValue, all methods use the Run->RunImpl base class functionality. How about the fact that the bookmark API also does things this way? On 2009/10/28 07:06:00, Aaron Boodman wrote: > It is weird to have this function called AsyncHistoryFunction when you already > have HistoryFunction which is deriving from AsyncFunction. > > It looks like in the two cases where you are using HistoryFunction they are > indeed synchronous cases, so it would be better to inherit from > SyncExtensionFunction not AsyncExtensionFunction. If you need to share code > between these two base classes you could do it with helper functions. http://codereview.chromium.org/313001/diff/18001/18014 File chrome/browser/extensions/extension_history_api_constants.cc (right): http://codereview.chromium.org/313001/diff/18001/18014#newcode9 Line 9: const wchar_t kAllHistoryKey[] = L"all_history"; On 2009/10/28 07:06:00, Aaron Boodman wrote: > allHistory Done. http://codereview.chromium.org/313001/diff/18001/18005 File chrome/common/extensions/api/extension_api.json (right): http://codereview.chromium.org/313001/diff/18001/18005#newcode1334 Line 1334: "title": {"type": "string", "optional": true, "description": "The text displayed for the node."}, On 2009/10/28 07:06:00, Aaron Boodman wrote: > "The title of the history page"? Sorry, this one slipped past me. Good suggestion. http://codereview.chromium.org/313001/diff/18001/18005#newcode1335 Line 1335: "lastVisitTime": {"type": "number", "optional": true, "description": "When this page was last loaded, represented in milliseconds since the epoch (new Date(dateAdded))."}, On 2009/10/28 07:06:00, Aaron Boodman wrote: > You can remove the parenthetical at the end of this. Done. http://codereview.chromium.org/313001/diff/18001/18005#newcode1349 Line 1349: "transition": {"type": "integer", "minimum": 0, "maximum": 10, "description": "The transition type for this visit from its referrer. The enumeration is: LINK=0, TYPED = 1, AUTO_BOOKMARK = 2, AUTO_SUBFRAME = 3, MANUAL_SUBFRAME = 4, GENERATED = 5, START_PAGE = 6, FORM_SUBMIT = 7, RELOAD = 8, KEYWORD = 9, KEYWORD_GENERATED = 10"} On 2009/10/28 07:06:00, Aaron Boodman wrote: > It would be cool to setup an actual enum for these values. You can do it > manually in extension_process_bindings.js, like: > > chrome.history.transitionType = { > LINK: 0, > ... > } Done. http://codereview.chromium.org/313001/diff/18001/18005#newcode1379 Line 1379: "name": "urlVisits", On 2009/10/28 07:06:00, Aaron Boodman wrote: > Method names should be verbs. How about getVisits()? Done. http://codereview.chromium.org/313001/diff/18001/18005#newcode1402 Line 1402: "name": "url", On 2009/10/28 07:06:00, Aaron Boodman wrote: > For forward flexibility, the parameter should be an object with one property, > 'url'. Also, I think it would make more sense to name this 'addVisit()'. Done. http://codereview.chromium.org/313001/diff/18001/18005#newcode1414 Line 1414: "name": "url", On 2009/10/28 07:06:00, Aaron Boodman wrote: > Again with an object with a single param here. Done. http://codereview.chromium.org/313001/diff/18001/18005#newcode1425 Line 1425: { On 2009/10/28 07:06:00, Aaron Boodman wrote: > The signature here should be a single object with multiple properties. Done. http://codereview.chromium.org/313001/diff/18001/18005#newcode1441 Line 1441: "name": "clear", On 2009/10/28 07:06:00, Aaron Boodman wrote: > For consistency with the above, how about calling this 'deleteAll()'? Done. A tossup between removeAll and deleteAll, but with the deleteURL and deleteRange methods consistancy recommends deleteAll. http://codereview.chromium.org/313001/diff/18001/18005#newcode1461 Line 1461: "name": "onRemoved", On 2009/10/28 07:06:00, Aaron Boodman wrote: > For consistency, I think this should be named 'onDeleted'. I would prefer to keep it onRemoved. While it can come from being deleted, it can also come from history expiring. Further, delete sounds very final for a URL. However the callback can occur for simply having a visit removed. The url still would be in the history and still appear in a subsequent search. http://codereview.chromium.org/313001/diff/18001/18005#newcode1469 Line 1469: "all_history": { "type": "boolean", "description": "True if all history was removed." }, On 2009/10/28 07:06:00, Aaron Boodman wrote: > camelCase. Done. http://codereview.chromium.org/313001/diff/18001/18005#newcode1470 Line 1470: "urls": { "type": "array", "items": { "type": "string" }, "optional": true} On 2009/10/28 07:06:00, Aaron Boodman wrote: > Couldn't this be a lot if a range was removed? Yes, but as with .search it will only be unique urls. It will not include all visits to each url. http://codereview.chromium.org/313001/diff/18001/18008 File chrome/test/data/extensions/api_test/history/test.js (right): http://codereview.chromium.org/313001/diff/18001/18008#newcode104 Line 104: function basic_search() { On 2009/10/28 07:06:00, Aaron Boodman wrote: > camelCase in JavaScript Done. http://codereview.chromium.org/313001/diff/18001/18008#newcode107 Line 107: waitAFewSeconds(0.5, pass(function() { On 2009/10/28 07:06:00, Aaron Boodman wrote: > Why are the timeouts required. These need to be removed; they will result in > flaky tests. Some timeouts are required to provide appropriate temporal space between actions (addition + deletion tests when deleting by a date range). One timeout is required for the full text search to update. I will make the tests more event driven, but there will always require some timeouts (although it should be a bug if certain things did not appear within a specified delta). http://codereview.chromium.org/313001/diff/18001/18008#newcode108 Line 108: var query = new Object; On 2009/10/28 07:06:00, Aaron Boodman wrote: > Nit: would be more javascripty to say: > > var query = { > search: '' > }; Done. I had removed all new Object(), but my search+replace missed new Object;.
Moving chrome.history to chrome.experimental.history. The changes to make this
happen are the replacement of history with experimental.history throughout and
code changes in
1) chrome/renderer/resources/extension_process_bindings.js
-- Allow hierarchical module definations from the .json file.
2) chrome/common/chrome_switches.{.h|cc}
-- Defines the switch.
3) chrome/common/extensions/extension.{.h|cc}
-- Enables the |experimental| module only if the
--enable-experimental-extension-api switch is present.
Almost there. Few more things. http://codereview.chromium.org/313001/diff/18001/18013 File chrome/browser/extensions/extension_history_api.cc (right): http://codereview.chromium.org/313001/diff/18001/18013#newcode190 Line 190: MessageLoop::current()->PostTask( On 2009/10/28 21:58:33, brg wrote: > On 2009/10/28 08:10:08, Aaron Boodman wrote: > > Why post a message to the current message loop here, instead of just calling > > SendResponse() immediately? > > Since I AddRef(), I must Release(). I can not Release() here, due to the > semantics of calling the history thread with a CancellableConsumer (which must > be acknowledged after the callback returns). So I must at least PostTask to > Release(). Ah, I didn't realize this constraint. Thanks. > But since I am PostTask'ing, I may as well be a good callback citizen and call > SendResponse from there in case the callback loop ever becomes serialized and > synchronous. Can you explain this paragraph a bit more? What case are you concerned about? http://codereview.chromium.org/313001/diff/18001/18016 File chrome/browser/extensions/extension_history_api.h (right): http://codereview.chromium.org/313001/diff/18001/18016#newcode72 Line 72: class AsyncHistoryFunction : public HistoryFunction { On 2009/10/28 21:58:33, brg wrote: > In the comment for the class, AsyncExtensionFunction is named as such because it > is asynchronous with respect to the browser thread. AsyncHistoryFunctions are > async with respect to their caller. So the term is being overloaded. Perhaps > HistoryFunctionWithCallback? > > But since addUrl and deleteUrl are indeed async to the browser thread should > they really be changed? They use the helper function GetUrlFromValue, all > methods use the Run->RunImpl base class functionality. How about the fact that > the bookmark API also does things this way? AsyncExtensionFunction is named that way because it calls back to the extension asynchronously wrt browser thread. That is, it can't compute its answer and reply completely on the browser thread, it needs to go to some other thread. In the case of addUrl and deleteUrl, you are correct that other threads are involved, but since those functions don't have callbacks its not really relevant. They could in fact be implemented using SyncExtensionFunction. === However, since addUrl and deleteUrl don't have callbacks it is probably simplest to just have one HistoryFunction that inherits AsyncExtensionFunction and use it for all your functions, including addUrl and deleteUrl. Since no callbacks are passed for addUrl and deleteUrl, that part of AsyncExtensionFunction will just be a no-op, and this will simplify things a bit in this code. http://codereview.chromium.org/313001/diff/18001/18005 File chrome/common/extensions/api/extension_api.json (right): http://codereview.chromium.org/313001/diff/18001/18005#newcode1461 Line 1461: "name": "onRemoved", On 2009/10/28 21:58:33, brg wrote: > On 2009/10/28 07:06:00, Aaron Boodman wrote: > > For consistency, I think this should be named 'onDeleted'. > > I would prefer to keep it onRemoved. While it can come from being deleted, it > can also come from history expiring. > > Further, delete sounds very final for a URL. However the callback can occur for > simply having a visit removed. The url still would be in the history and still > appear in a subsequent search. I see. In that case, why is this event useful? It tells you that some visits for a URL have been removed, but not which ones. If we have this, maybe it should be onHistoryRemoved or onVisitsRemoved, to clarify what is being removed (since there are multiple objects in the history module). http://codereview.chromium.org/313001/diff/28001/29019 File chrome/common/chrome_switches.cc (right): http://codereview.chromium.org/313001/diff/28001/29019#newcode168 Line 168: const char kEnableExperimentalExtensionApi[] = Nit: can you add an "s" to the end of the constant name to match the flag? http://codereview.chromium.org/313001/diff/28001/29016 File chrome/renderer/resources/extension_process_bindings.js (right): http://codereview.chromium.org/313001/diff/28001/29016#newcode239 Line 239: for (var index = 0; index < namespaces.length; ++index) { Nit: there is a javascriptism you can use here to make this a bit more readable: for (var i = 0, name; name = namespaces[i]; i++) { } Also, in JavaScript, we use i++ in loops, not ++i. http://codereview.chromium.org/313001/diff/28001/29007 File chrome/test/data/extensions/api_test/history/test.js (right): http://codereview.chromium.org/313001/diff/28001/29007#newcode28 Line 28: * As per the JS guidelines this is a global object. Out of curiosity: which JS guidelines are you referring to? I'm not familiar with this one. http://codereview.chromium.org/313001/diff/28001/29007#newcode52 Line 52: * An object used for listening to the chrome.history.onRemoved events.The global Nit: add space before "The".
Changed. Update 13 has the requested changes. Update 14 is a single type in the description string. http://codereview.chromium.org/313001/diff/18001/18013 File chrome/browser/extensions/extension_history_api.cc (right): http://codereview.chromium.org/313001/diff/18001/18013#newcode190 Line 190: MessageLoop::current()->PostTask( > Can you explain this paragraph a bit more? What case are you concerned about? It is always good practice, in an event driven framework, to do as little work in a callback as practical. So with having to PostTask anyhow, I moved the SendResult to the next event. http://codereview.chromium.org/313001/diff/18001/18016 File chrome/browser/extensions/extension_history_api.h (right): http://codereview.chromium.org/313001/diff/18001/18016#newcode72 Line 72: class AsyncHistoryFunction : public HistoryFunction { On 2009/10/29 18:17:48, Aaron Boodman wrote: > On 2009/10/28 21:58:33, brg wrote: > > In the comment for the class, AsyncExtensionFunction is named as such because > it > > is asynchronous with respect to the browser thread. AsyncHistoryFunctions are > > async with respect to their caller. So the term is being overloaded. Perhaps > > HistoryFunctionWithCallback? > > > > But since addUrl and deleteUrl are indeed async to the browser thread should > > they really be changed? They use the helper function GetUrlFromValue, all > > methods use the Run->RunImpl base class functionality. How about the fact > that > > the bookmark API also does things this way? > > AsyncExtensionFunction is named that way because it calls back to the extension > asynchronously wrt browser thread. That is, it can't compute its answer and > reply completely on the browser thread, it needs to go to some other thread. > > In the case of addUrl and deleteUrl, you are correct that other threads are > involved, but since those functions don't have callbacks its not really > relevant. They could in fact be implemented using SyncExtensionFunction. > > === > > However, since addUrl and deleteUrl don't have callbacks it is probably simplest > to just have one HistoryFunction that inherits AsyncExtensionFunction and use it > for all your functions, including addUrl and deleteUrl. > > Since no callbacks are passed for addUrl and deleteUrl, that part of > AsyncExtensionFunction will just be a no-op, and this will simplify things a bit > in this code. Done. http://codereview.chromium.org/313001/diff/18001/18005 File chrome/common/extensions/api/extension_api.json (right): http://codereview.chromium.org/313001/diff/18001/18005#newcode1461 Line 1461: "name": "onRemoved", I find it useful as a notification that things are change. But you are right, the use case of getting the notification and then requiring a query for the visits is problematic. I think that this requires a change to the history service to provide the visit-id removed in the notification. Until then though, it would be nice to get some feedback from users in chrome.experimental. On 2009/10/29 18:17:48, Aaron Boodman wrote: > On 2009/10/28 21:58:33, brg wrote: > > On 2009/10/28 07:06:00, Aaron Boodman wrote: > > > For consistency, I think this should be named 'onDeleted'. > > > > I would prefer to keep it onRemoved. While it can come from being deleted, it > > can also come from history expiring. > > > > Further, delete sounds very final for a URL. However the callback can occur > for > > simply having a visit removed. The url still would be in the history and > still > > appear in a subsequent search. > > I see. In that case, why is this event useful? It tells you that some visits for > a URL have been removed, but not which ones. > > If we have this, maybe it should be onHistoryRemoved or onVisitsRemoved, to > clarify what is being removed (since there are multiple objects in the history > module). http://codereview.chromium.org/313001/diff/28001/29019 File chrome/common/chrome_switches.cc (right): http://codereview.chromium.org/313001/diff/28001/29019#newcode168 Line 168: const char kEnableExperimentalExtensionApi[] = On 2009/10/29 18:17:49, Aaron Boodman wrote: > Nit: can you add an "s" to the end of the constant name to match the flag? Done. http://codereview.chromium.org/313001/diff/28001/29016 File chrome/renderer/resources/extension_process_bindings.js (right): http://codereview.chromium.org/313001/diff/28001/29016#newcode239 Line 239: for (var index = 0; index < namespaces.length; ++index) { On 2009/10/29 18:17:49, Aaron Boodman wrote: > Nit: there is a javascriptism you can use here to make this a bit more readable: > > for (var i = 0, name; name = namespaces[i]; i++) { > } > > Also, in JavaScript, we use i++ in loops, not ++i. Done. Neat. http://codereview.chromium.org/313001/diff/28001/29007 File chrome/test/data/extensions/api_test/history/test.js (right): http://codereview.chromium.org/313001/diff/28001/29007#newcode28 Line 28: * As per the JS guidelines this is a global object. On 2009/10/29 18:17:49, Aaron Boodman wrote: > Out of curiosity: which JS guidelines are you referring to? I'm not familiar > with this one. It is called out in a document called "Notes on Javascript readability." Looking now it seems as if it was a strong recommendation and not a requirement. I'll remove that statement from this file. http://codereview.chromium.org/313001/diff/28001/29007#newcode52 Line 52: * An object used for listening to the chrome.history.onRemoved events.The global On 2009/10/29 18:17:49, Aaron Boodman wrote: > Nit: add space before "The". Done.
On 2009/10/29 23:30:20, brg wrote: > Changed. I'm not seeing the change to combine AsyncHistoryFunction and HistoryFunction. I think you could just have everything inherit AsyncHistoryFunction. When you call SendResponse() AsyncFunction will just do nothing if there is no callback, which will be the case for addUrl and deleteUrl.
Also, one last thing I just remembered. Can you update the docs? You can do this by building test_shell and running chrome/common/extensions/docs/build/build.bat (or build.py on mac and linux). This will create pretty docs from the JSON that you can browse. There won't be a link from the main nav since this is experimental, but if you know the secret URL, you will be able to load them.
As discuss, AsyncHistoryFunction has been renamed to HistoryFunctionWithCallback. Documentation is blocked by a bug in layoutTestController that will be investigated later.
LGTM Thanks for the contribution. |
