Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
201393002:
Add onChanged callback for chrome.sessions API. (Closed)
|
Created:
6 years, 9 months ago by wjywbs Modified:
6 years, 8 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd onChanged callback for chrome.sessions API.
Currently there is no method to receive the changes of recently closed pages
in chrome.sessions APIs. This callback can give extensions a chance to know
there is a change and update by calling GetRecentlyClosed again.
R=kalman@chromium.org
BUG=353007
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260464
Patch Set 1 #
Total comments: 5
Patch Set 2 : rename to onChanged, rebase #
Total comments: 12
Patch Set 3 : kalman's comment, rebase #Patch Set 4 : Update test #
Total comments: 3
Patch Set 5 : #Patch Set 6 : Use EventRouter::Get #
Total comments: 1
Patch Set 7 : #Patch Set 8 : #
Total comments: 1
Patch Set 9 : #
Messages
Total messages: 51 (0 generated)
Hello kalman, I read the "API Proposals (New APIs Start Here)" wiki, but I am not sure the process of adding features to existing apis. I posted the reasons why this callback would be helpful in the issue. Thanks.
https://codereview.chromium.org/201393002/diff/1/chrome/common/extensions/api... File chrome/common/extensions/api/sessions.json (right): https://codereview.chromium.org/201393002/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/sessions.json:110: "events": [ Cool, we've wanted to add this sort of functionality for a while. We will probably want to include some data about the sessions which changed in here. However, I can't find a reference to a proposal for this API. Typically you would fill out the API proposal on that page (it's a public document) explaining the use case for this method and so on. A number of the questions won't apply to you but the most important part is proposing the new event. https://codereview.chromium.org/201393002/diff/1/chrome/test/data/extensions/... File chrome/test/data/extensions/api_test/sessions/sessions.js (right): https://codereview.chromium.org/201393002/diff/1/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/sessions/sessions.js:111: var expectedCallbackCount = 2, callbackCount = 0; what does this actually test?
Replied comments. Thanks. https://codereview.chromium.org/201393002/diff/1/chrome/common/extensions/api... File chrome/common/extensions/api/sessions.json (right): https://codereview.chromium.org/201393002/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/sessions.json:110: "events": [ On 2014/03/17 14:21:18, kalman wrote: > Cool, we've wanted to add this sort of functionality for a while. > > We will probably want to include some data about the sessions which changed in > here. > > However, I can't find a reference to a proposal for this API. Typically you > would fill out the API proposal on that page (it's a public document) explaining > the use case for this method and so on. > > A number of the questions won't apply to you but the most important part is > proposing the new event. Sure, I will write the proposal tonight. https://codereview.chromium.org/201393002/diff/1/chrome/test/data/extensions/... File chrome/test/data/extensions/api_test/sessions/sessions.js (right): https://codereview.chromium.org/201393002/diff/1/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/sessions/sessions.js:111: var expectedCallbackCount = 2, callbackCount = 0; On 2014/03/17 14:21:18, kalman wrote: > what does this actually test? Lines 63-79 of this file describe that this retrieveClosedTabs function will close 2 tabs, so chrome.sessions.onRecentlyClosed will be triggered twice.
https://codereview.chromium.org/201393002/diff/1/chrome/common/extensions/api... File chrome/common/extensions/api/sessions.json (right): https://codereview.chromium.org/201393002/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/sessions.json:110: "events": [ On 2014/03/17 19:27:24, wjywbs wrote: > On 2014/03/17 14:21:18, kalman wrote: > > Cool, we've wanted to add this sort of functionality for a while. > > > > We will probably want to include some data about the sessions which changed in > > here. > > > > However, I can't find a reference to a proposal for this API. Typically you > > would fill out the API proposal on that page (it's a public document) > explaining > > the use case for this method and so on. > > > > A number of the questions won't apply to you but the most important part is > > proposing the new event. > > Sure, I will write the proposal tonight. Ok, let's resolve my comments in there. Then ping this CL to me again.
Friendly ping. I’ve renamed the event name to onChanged. PTAL, thanks.
https://codereview.chromium.org/201393002/diff/20001/chrome/common/extensions... File chrome/common/extensions/api/sessions.json (right): https://codereview.chromium.org/201393002/diff/20001/chrome/common/extensions... chrome/common/extensions/api/sessions.json:113: "description": "Fired when recently closed tabs and/or windows are changed.", make sure you specify that this is local session only, not foreign https://codereview.chromium.org/201393002/diff/20001/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/sessions/sessions.js (right): https://codereview.chromium.org/201393002/diff/20001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/sessions/sessions.js:72: } this doesn't actually make any assertions. yes it counts but what then? does it mean that the test times out? that's not a great experience. what if it's called too many times? testing events is hard :\ an alternatively might be to add a listener for each test then do like assertEq(2, numEventsFired) or whatever.
sent comment too early. https://codereview.chromium.org/201393002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/sessions/sessions_api.cc (right): https://codereview.chromium.org/201393002/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/sessions/sessions_api.cc:582: : profile_(profile), tab_restore_service_(NULL) { just construct with the right value for tab_store_service_: : profile_(profile), tab_restore_service_(TabRestoreServiceFactory::GetForProfile(profile)) https://codereview.chromium.org/201393002/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/sessions/sessions_api.cc:590: // shouldn't be loaded. this comment is already on the interface https://codereview.chromium.org/201393002/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/sessions/sessions_api.cc:606: ExtensionSystem::Get(profile_)->event_router()->BroadcastEvent(event.Pass()); consider inlining |event|, but up to you: ExtensionSystem::Get(profile_)->event_router()->BroadcastEvent( make_scoped_ptr( new Event(api::sessions::OnChanged::kEventName, args.Pass())) https://codereview.chromium.org/201393002/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/sessions/sessions_api.cc:619: api::sessions::OnChanged::kEventName); since you're inlining this in Shutdown() (I like inlining) do so here as well https://codereview.chromium.org/201393002/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/sessions/sessions_api.cc:641: ExtensionSystem::Get(browser_context_)->event_router()->UnregisterObserver( I don't understand this. Why unregister when a listener is added? because there's only 1 listener? I'm not familiar with the idiom; yoz@ ? https://codereview.chromium.org/201393002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/sessions/sessions_api.h (right): https://codereview.chromium.org/201393002/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/sessions/sessions_api.h:95: // is destroyed. these are just overrides. the idiom is: // TabRestoreServiceObserver implementation. virtual void TabRestoreServiceChanged(..) OVERRIDE; virtual void TabRestoreServiceDestroyed(..) OVERRIDE;
https://codereview.chromium.org/201393002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/sessions/sessions_api.cc (right): https://codereview.chromium.org/201393002/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/sessions/sessions_api.cc:641: ExtensionSystem::Get(browser_context_)->event_router()->UnregisterObserver( On 2014/03/21 11:03:06, kalman wrote: > I don't understand this. Why unregister when a listener is added? because > there's only 1 listener? I'm not familiar with the idiom; yoz@ ? Basically you lazily register with EventRouter when you get your first listener. After that you don't need to re-register for additional listeners.
https://codereview.chromium.org/201393002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/sessions/sessions_api.cc (right): https://codereview.chromium.org/201393002/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/sessions/sessions_api.cc:641: ExtensionSystem::Get(browser_context_)->event_router()->UnregisterObserver( On 2014/03/21 15:56:02, Yoyo Zhou wrote: > On 2014/03/21 11:03:06, kalman wrote: > > I don't understand this. Why unregister when a listener is added? because > > there's only 1 listener? I'm not familiar with the idiom; yoz@ ? > > Basically you lazily register with EventRouter when you get your first listener. > After that you don't need to re-register for additional listeners. Ah I see.
Modified per kalman's comment. I copied a lot of existing code from recently_closed_tabs_handler.cc and management_api.cc and I didn't do any clean up in the first CL. PTAL. Thanks. https://codereview.chromium.org/201393002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/sessions/sessions_api.h (right): https://codereview.chromium.org/201393002/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/sessions/sessions_api.h:95: // is destroyed. On 2014/03/21 11:03:06, kalman wrote: > these are just overrides. the idiom is: > > // TabRestoreServiceObserver implementation. > virtual void TabRestoreServiceChanged(..) OVERRIDE; > virtual void TabRestoreServiceDestroyed(..) OVERRIDE; Should I remove these comments? https://codereview.chromium.org/201393002/diff/20001/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/sessions/sessions.js (right): https://codereview.chromium.org/201393002/diff/20001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/sessions/sessions.js:72: } On 2014/03/21 10:52:59, kalman wrote: > this doesn't actually make any assertions. yes it counts but what then? does it > mean that the test times out? that's not a great experience. what if it's called > too many times? > > testing events is hard :\ > > an alternatively might be to add a listener for each test then do like > > assertEq(2, numEventsFired) > > or whatever. I haven't found a good way yet. I looked at the other api tests just now, such as chrome.tabs.onRemoved event: chrome/test/data/extensions/api_test/tabs/on_removed/test.js In that test, it used chrome.test.listenOnce, so if the callback is called for multiple times, it would not detect that. Yes, it times out if the listener is not called. If we do assertEq(2, numEventsFired), I think I need to wait for 1 or 2 seconds for finishing the callbacks and then do this check. Is this acceptable?
On 2014/03/21 19:45:32, wjywbs wrote: > Modified per kalman's comment. > > I copied a lot of existing code from recently_closed_tabs_handler.cc and > management_api.cc and I didn't do any clean up in the first CL. > > PTAL. Thanks. > > https://codereview.chromium.org/201393002/diff/20001/chrome/browser/extension... > File chrome/browser/extensions/api/sessions/sessions_api.h (right): > > https://codereview.chromium.org/201393002/diff/20001/chrome/browser/extension... > chrome/browser/extensions/api/sessions/sessions_api.h:95: // is destroyed. > On 2014/03/21 11:03:06, kalman wrote: > > these are just overrides. the idiom is: > > > > // TabRestoreServiceObserver implementation. > > virtual void TabRestoreServiceChanged(..) OVERRIDE; > > virtual void TabRestoreServiceDestroyed(..) OVERRIDE; > Should I remove these comments? > > https://codereview.chromium.org/201393002/diff/20001/chrome/test/data/extensi... > File chrome/test/data/extensions/api_test/sessions/sessions.js (right): > > https://codereview.chromium.org/201393002/diff/20001/chrome/test/data/extensi... > chrome/test/data/extensions/api_test/sessions/sessions.js:72: } > On 2014/03/21 10:52:59, kalman wrote: > > this doesn't actually make any assertions. yes it counts but what then? does > it > > mean that the test times out? that's not a great experience. what if it's > called > > too many times? > > > > testing events is hard :\ > > > > an alternatively might be to add a listener for each test then do like > > > > assertEq(2, numEventsFired) > > > > or whatever. > > I haven't found a good way yet. I looked at the other api tests just now, such > as chrome.tabs.onRemoved event: > chrome/test/data/extensions/api_test/tabs/on_removed/test.js > In that test, it used chrome.test.listenOnce, so if the callback is called for > multiple times, it would not detect that. Yes, it times out if the listener is > not called. > > If we do assertEq(2, numEventsFired), I think I need to wait for 1 or 2 seconds > for finishing the callbacks and then do this check. Is this acceptable? Don't worry about waiting N seconds but do wait until at least that number of events have arrived.
I updated the testing method. PTAL. Thanks.
https://codereview.chromium.org/201393002/diff/60001/chrome/browser/extension... File chrome/browser/extensions/api/sessions/sessions_api.cc (right): https://codereview.chromium.org/201393002/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/sessions/sessions_api.cc:607: ExtensionSystem::Get(profile_)->event_router()->BroadcastEvent( You can now use just EventRouter::Get(profile_) here and everywhere. https://codereview.chromium.org/201393002/diff/60001/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/sessions/sessions.js (right): https://codereview.chromium.org/201393002/diff/60001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/sessions/sessions.js:65: chrome.sessions.onChanged, I keep reading this as a function body, because the indent is 2 not 4. I suggest changing this to look like var done = chrome.test.listenForever(chrome.sessions.onChanged, function() { callbackCount++; }); https://codereview.chromium.org/201393002/diff/60001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/sessions/sessions.js:79: window.setTimeout(checkEvent, 1000); It would be nice to notify the user why this test is still alive, if it is. E.g. log each time the check is re-retried. Really this should be a helper exposed on chrome.test, like chrome.test.listenNTimes, but let's worry about that later (perhaps file a bug). Anyway, it's not just as simple as logging "checking" each time you check, because that will cause a lot of log spew. There needs to be a way to signal to the event checker that all the events should have happened by now, and to wait. How about: - change the test retry to 1000 (from 100) - make this function return a function meaning "test is finished, call me now" - when that function is called, start logging "waiting for <n> more onChanged events"
I updated the test and used EventRouter::Get. PTAL. Thanks.
nice! lgtm. https://codereview.chromium.org/201393002/diff/100001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/sessions/sessions.js (right): https://codereview.chromium.org/201393002/diff/100001/chrome/test/data/extens... chrome/test/data/extensions/api_test/sessions/sessions.js:85: window.setTimeout(checkEvent, 100); 100*10ms is too small, this only gives the test 1 second to finish which is going to be flaky. I'd say give it 10 seconds, or really, just don't have a max number of retries at all and just let the normal test timeout (30s) kick in. So yeah. No maxRetry. and increase to 1s so that while it's waiting it doesn't totally spam the console output.
I removed the retry and increased the retry wait time to 300ms. In my testing, it almost always finishes checking within 100ms, and I think the console log is not shown in buildbot if the test passes. Thanks.
On 2014/03/28 18:25:26, wjywbs wrote: > I removed the retry and increased the retry wait time to 300ms. In my testing, > it almost always finishes checking within 100ms, and I think the console log is > not shown in buildbot if the test passes. Thanks. Ok. Maybe you can do 100ms checking then, but only log every 10 retries?
Sure. PTAL and thanks.
lgtm https://codereview.chromium.org/201393002/diff/120001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/sessions/sessions.js (right): https://codereview.chromium.org/201393002/diff/120001/chrome/test/data/extens... chrome/test/data/extensions/api_test/sessions/sessions.js:78: window.setTimeout(checkEvent, 100); last nit: declare this 100 as a var up in the checkOnChangedEvent scope. // The frequency in ms between checking whether the right events have // fired. Every 10 attempts progress is logged. var retryPeriod = 100ms;
Sure. PTAL.
lgtm
The CQ bit was checked by wjywbs@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wjywbs@gmail.com/201393002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
The CQ bit was checked by wjywbs@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wjywbs@gmail.com/201393002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
The CQ bit was checked by wjywbs@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wjywbs@gmail.com/201393002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
The CQ bit was checked by wjywbs@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wjywbs@gmail.com/201393002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
The CQ bit was checked by wjywbs@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wjywbs@gmail.com/201393002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
The CQ bit was checked by wjywbs@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wjywbs@gmail.com/201393002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
The CQ bit was checked by wjywbs@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wjywbs@gmail.com/201393002/140001
The CQ bit was unchecked by wjywbs@gmail.com
The CQ bit was checked by wjywbs@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wjywbs@gmail.com/201393002/140001
Message was sent while issue was closed.
Change committed as 260464 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
