Sessions API - previously Session Restore API. Supports restoring currently open foreign windows an…

Sessions API - previously Session Restore API. Supports restoring currently open foreign windows and tabs. API proposal accessible at go/sessions-apidoc. SyncedDevices / DeviceInfo still needs to be added in once they are written and checked in by chrome.syncedDevices developer. Session name is currently holding the place of DeviceInfo in "info" component of Session object (sessions.json). An "on change" event listener for changes in device sync needs to be added as well.

BUG=123266
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=219066

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=219187

[Kristen Dwan, 2013-07-31 19:46:16]

Sorry all for the lengthy review ... couldn't come up with a straight forward
way to break it up. Thanks!

[not at google - send to devlin, 2013-07-31 22:54:22]

https://codereview.chromium.org/21022018/diff/4001/chrome/browser/extensions/...
File chrome/browser/extensions/api/sessions/sessions_api.h (right):

https://codereview.chromium.org/21022018/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/api/sessions/sessions_api.h:3: // found in the LICENSE
file.
could you make git detect this and the next 2 files as a move?

it's close enough, right?

https://codereview.chromium.org/21022018/diff/4001/chrome/common/extensions/a...
File chrome/common/extensions/api/tabs.json (right):

https://codereview.chromium.org/21022018/diff/4001/chrome/common/extensions/a...
chrome/common/extensions/api/tabs.json:14: "id": {"type": "integer", "minimum":
0, "description": "The ID of the tab. Tab IDs are unique within a browser
session."},
how is this not giving errors, it should be optional?

https://codereview.chromium.org/21022018/diff/4001/chrome/common/extensions/a...
File chrome/common/extensions/api/windows.json (right):

https://codereview.chromium.org/21022018/diff/4001/chrome/common/extensions/a...
chrome/common/extensions/api/windows.json:17: "id": {"type": "integer",
"optional": true, "minimum": 0, "description": "The ID of the window. Window IDs
are unique within a browser session. Under some circumstances a Window may not
be assigned an ID, for example when querying windows from the $ref:sessions
API."},
... in which case sessionId may be present.

https://codereview.chromium.org/21022018/diff/4001/chrome/common/extensions/a...
chrome/common/extensions/api/windows.json:34: "description": "The state of this
browser window. Under some circumstances a Window may not be assigned state
property, for example when querying closed windows from the $ref:sessions API.",
all of these references to the sessions API we should actually delete and make
them _not_ optional - give them either stub or their old values.

https://codereview.chromium.org/21022018/diff/4001/chrome/test/data/extension...
File chrome/test/data/extensions/api_test/sessions/sessions.js (right):

https://codereview.chromium.org/21022018/diff/4001/chrome/test/data/extension...
chrome/test/data/extensions/api_test/sessions/sessions.js:4: 
IIRC this test is disabled on most platforms, because it takes waaaaay too long
to run. It's not high quality; really needs to be broken up into much smaller
tests and factored in a sane way.

[sky, 2013-07-31 23:58:58]

two meta comments for you: if you can, split this review into chunks. Generally
smaller reviews go faster. Similarly, when you have a ginormous review make it
clear who is suppose to review what.

[sky, 2013-08-01 00:05:42]

How about some test coverage here too?

https://codereview.chromium.org/21022018/diff/4001/chrome/browser/sessions/pe...
File chrome/browser/sessions/persistent_tab_restore_service.h (right):

https://codereview.chromium.org/21022018/diff/4001/chrome/browser/sessions/pe...
chrome/browser/sessions/persistent_tab_restore_service.h:34: // Returns the
WebContents of the restored tab if entry is a Tab. In the case
Don't duplicate the documentation when overriding interfaces. See all the other
methods here that don't copy existing docs.

https://codereview.chromium.org/21022018/diff/4001/chrome/browser/sessions/se...
File chrome/browser/sessions/session_restore.cc (right):

https://codereview.chromium.org/21022018/diff/4001/chrome/browser/sessions/se...
chrome/browser/sessions/session_restore.cc:570: // Restore window(s) from a
foreign session. Return corresponding created
Returns newly created Browsers.

https://codereview.chromium.org/21022018/diff/4001/chrome/browser/sessions/se...
chrome/browser/sessions/session_restore.cc:608: WindowOpenDisposition
disposition) {
nit: fix indentation.

https://codereview.chromium.org/21022018/diff/4001/chrome/browser/sessions/se...
File chrome/browser/sessions/session_restore.h (right):

https://codereview.chromium.org/21022018/diff/4001/chrome/browser/sessions/se...
chrome/browser/sessions/session_restore.h:58: // created Browsers are returned
to identify the restored Windows.
You don't need the last part: 'Returns the created Browsers' is fine.

https://codereview.chromium.org/21022018/diff/4001/chrome/browser/sessions/ta...
File chrome/browser/sessions/tab_restore_service.h (right):

https://codereview.chromium.org/21022018/diff/4001/chrome/browser/sessions/ta...
chrome/browser/sessions/tab_restore_service.h:160: // restoring type Window,
returns the WebContents of the last successfully
Return value doesn't make sense here. SHouldn't this be a vector? If the caller
only cares about the last restored tab in the case of a window it can use
back().

https://codereview.chromium.org/21022018/diff/4001/chrome/browser/sessions/ta...
chrome/browser/sessions/tab_restore_service.h:177: // of restoring type Window,
returns the WebContents of the last successfully
Same comment.

https://codereview.chromium.org/21022018/diff/4001/chrome/browser/sessions/ta...
File chrome/browser/sessions/tab_restore_service_helper.h (right):

https://codereview.chromium.org/21022018/diff/4001/chrome/browser/sessions/ta...
chrome/browser/sessions/tab_restore_service_helper.h:82: // In the case of
restoring a window, the returned WebContents will contain
Nuke comments since it's just duplicating what you already have.

[Kristen Dwan, 2013-08-02 20:54:19]

moved session code returning webcontents to its own cl to break it down a bit.
added similarity detection for api files. should help. thanks.

https://codereview.chromium.org/21022018/diff/4001/chrome/browser/extensions/...
File chrome/browser/extensions/api/sessions/sessions_api.h (right):

https://codereview.chromium.org/21022018/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/api/sessions/sessions_api.h:3: // found in the LICENSE
file.
On 2013/07/31 22:54:22, kalman wrote:
> could you make git detect this and the next 2 files as a move?
> 
> it's close enough, right?

Done.

https://codereview.chromium.org/21022018/diff/4001/chrome/browser/sessions/pe...
File chrome/browser/sessions/persistent_tab_restore_service.h (right):

https://codereview.chromium.org/21022018/diff/4001/chrome/browser/sessions/pe...
chrome/browser/sessions/persistent_tab_restore_service.h:34: // Returns the
WebContents of the restored tab if entry is a Tab. In the case
On 2013/08/01 00:05:43, sky wrote:
> Don't duplicate the documentation when overriding interfaces. See all the
other
> methods here that don't copy existing docs.

Done.

https://codereview.chromium.org/21022018/diff/4001/chrome/browser/sessions/se...
File chrome/browser/sessions/session_restore.cc (right):

https://codereview.chromium.org/21022018/diff/4001/chrome/browser/sessions/se...
chrome/browser/sessions/session_restore.cc:570: // Restore window(s) from a
foreign session. Return corresponding created
On 2013/08/01 00:05:43, sky wrote:
> Returns newly created Browsers.

Done.

https://codereview.chromium.org/21022018/diff/4001/chrome/browser/sessions/se...
chrome/browser/sessions/session_restore.cc:608: WindowOpenDisposition
disposition) {
On 2013/08/01 00:05:43, sky wrote:
> nit: fix indentation.

Done.

https://codereview.chromium.org/21022018/diff/4001/chrome/browser/sessions/se...
File chrome/browser/sessions/session_restore.h (right):

https://codereview.chromium.org/21022018/diff/4001/chrome/browser/sessions/se...
chrome/browser/sessions/session_restore.h:58: // created Browsers are returned
to identify the restored Windows.
On 2013/08/01 00:05:43, sky wrote:
> You don't need the last part: 'Returns the created Browsers' is fine.

Done.

https://codereview.chromium.org/21022018/diff/4001/chrome/browser/sessions/ta...
File chrome/browser/sessions/tab_restore_service.h (right):

https://codereview.chromium.org/21022018/diff/4001/chrome/browser/sessions/ta...
chrome/browser/sessions/tab_restore_service.h:160: // restoring type Window,
returns the WebContents of the last successfully
On 2013/08/01 00:05:43, sky wrote:
> Return value doesn't make sense here. SHouldn't this be a vector? If the
caller
> only cares about the last restored tab in the case of a window it can use
> back().

Done.

https://codereview.chromium.org/21022018/diff/4001/chrome/browser/sessions/ta...
chrome/browser/sessions/tab_restore_service.h:177: // of restoring type Window,
returns the WebContents of the last successfully
On 2013/08/01 00:05:43, sky wrote:
> Same comment.

Done.

https://codereview.chromium.org/21022018/diff/4001/chrome/browser/sessions/ta...
File chrome/browser/sessions/tab_restore_service_helper.h (right):

https://codereview.chromium.org/21022018/diff/4001/chrome/browser/sessions/ta...
chrome/browser/sessions/tab_restore_service_helper.h:82: // In the case of
restoring a window, the returned WebContents will contain
On 2013/08/01 00:05:43, sky wrote:
> Nuke comments since it's just duplicating what you already have.

Done.

https://codereview.chromium.org/21022018/diff/4001/chrome/common/extensions/a...
File chrome/common/extensions/api/tabs.json (right):

https://codereview.chromium.org/21022018/diff/4001/chrome/common/extensions/a...
chrome/common/extensions/api/tabs.json:14: "id": {"type": "integer", "minimum":
0, "description": "The ID of the tab. Tab IDs are unique within a browser
session."},
On 2013/07/31 22:54:22, kalman wrote:
> how is this not giving errors, it should be optional?

not sure why that didn't give errors .... fixed tho.

https://codereview.chromium.org/21022018/diff/4001/chrome/common/extensions/a...
File chrome/common/extensions/api/windows.json (right):

https://codereview.chromium.org/21022018/diff/4001/chrome/common/extensions/a...
chrome/common/extensions/api/windows.json:34: "description": "The state of this
browser window. Under some circumstances a Window may not be assigned state
property, for example when querying closed windows from the $ref:sessions API.",
On 2013/07/31 22:54:22, kalman wrote:
> all of these references to the sessions API we should actually delete and make
> them _not_ optional - give them either stub or their old values.

okay - info isn't stored for old top/left/width/height, so just give them 0 (?)
and add "closed" for state?

https://codereview.chromium.org/21022018/diff/4001/chrome/test/data/extension...
File chrome/test/data/extensions/api_test/sessions/sessions.js (right):

https://codereview.chromium.org/21022018/diff/4001/chrome/test/data/extension...
chrome/test/data/extensions/api_test/sessions/sessions.js:4: 
On 2013/07/31 22:54:22, kalman wrote:
> IIRC this test is disabled on most platforms, because it takes waaaaay too
long
> to run. It's not high quality; really needs to be broken up into much smaller
> tests and factored in a sane way.

Alright, will refactor and send out separately.

[not at google - send to devlin, 2013-08-06 17:22:44]

https://codereview.chromium.org/21022018/diff/4001/chrome/common/extensions/a...
File chrome/common/extensions/api/windows.json (right):

https://codereview.chromium.org/21022018/diff/4001/chrome/common/extensions/a...
chrome/common/extensions/api/windows.json:34: "description": "The state of this
browser window. Under some circumstances a Window may not be assigned state
property, for example when querying closed windows from the $ref:sessions API.",
On 2013/08/02 20:54:19, Kristen Dwan wrote:
> On 2013/07/31 22:54:22, kalman wrote:
> > all of these references to the sessions API we should actually delete and
make
> > them _not_ optional - give them either stub or their old values.
> 
> okay - info isn't stored for old top/left/width/height, so just give them 0
(?)
> and add "closed" for state?

Yep, closed makes sense.

For the top/left/width/height either see what minimised does and use that - it
probably uses the old height which is no good - better might be wherever the
place that it would be restored. Hopefully you can access that information.

[not at google - send to devlin, 2013-08-06 19:04:09]

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
File chrome/browser/extensions/api/sessions/sessions_api.cc (right):

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:56: }
Rather than sharing a magic variable (kForeignIdSeparator) and having util
classes (those 3), it would be nicer to encapsulate a foreign ID within a little
class, providing ToString() and FromString() methods.

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:67:
extensions::WindowController** controller) {
Please pull this into a common place and share it with the windows API code:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:118: const
TabRestoreService::Tab& tab, const int* const session_id,
The second const isn't necessary.

That said I think it should be the job of the caller to decide what the session
ID is. Pass it in directly. Callers can pass in tab.id if they want.

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:234:
scoped_ptr<tabs::Tab> SessionsGetDevicesFunction::CreateTabModel(
what's the difference between this and the other CreateTabModel?

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:258:
scoped_ptr<windows::Window> SessionsGetDevicesFunction::CreateWindowModel(
ditto?

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:313:
SessionsGetDevicesFunction::CreateSessionModel(
ditto?

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:339:
SetError(kSessionsSyncNotEnabledError);
Not sure about this one. There should be a way for extensions to know whether
session sync is enabled, but can't be via an error message. That canonicalises
error messages, which is *not* what we want.

I.e. as an extension author, how am I supposed to tell? String-compare error
messages? Not good.

If only we had documented and strict error codes in APIs - but we don't. It's a
shame.

Here are some options:

1 - have another API call, chrome.sessions.canGetDevices(...). Ugh.

2 - make devices null if session sync isn't enabled. A bit bug-inducing since
extensions that go "if (result.devices)" wouldn't be able to tell between an
empty list and null. Obviously that's a bug in the extension, but it does imply
a confusing API.

3 - make results an object rather than an array. too much nesting, yuck.

Of those, (2) is by far the most preferable. Any other ideas?

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:342: unsigned int
max_results = kMaxSyncedSessionResults;
just "int".

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:344:
GetDevices::Params::Create(*args_));
1 line?

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:359:
SetError(kSyncedSessionsListEmptyError);
This isn't really an *error* case. Similar as before.

Speaking of which, all of these corner-ish cases, we should document in some
way.

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:385:
SetError(kRecentlyClosedListEmptyError);
also not an error - and this function shouldn't be returning false (so -
existing bug).

hm, we should have a CHECK somewhere in the extension system that "return false"
also sets error_. Anyhow.

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:389: bool is_window =
entries.front()->type == TabRestoreService::WINDOW;
inline this?

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:393:
SetError(kReturnObjectError);
I think we can do a better job of an error message here.

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:398: WindowController*
controller;
always initialise pointers to NULL in situations like these.

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:402: return false;
error message

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:415:
browser->tab_strip_model()->GetActiveWebContents());
delegate isn't used until way later, grab it then.

and is it really the case that it can't be NULL?

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:427: bool is_window =
false;
rather than hold onto these two values, perhaps find a pointer to the entry.
checks for is_valid_id would be replaced by entry != NULL. CHeck for is_window
would be directly inspecting the type.

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:461:
SetError(kReturnObjectError);
yeah so kReturnObject error - not a great error in general

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:480: const std::string&
session_tag, int id, Browser* browser) {
similar comments throughout here and the next function

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:504: } else {
no else after return

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
File chrome/browser/extensions/api/sessions/sessions_api.h (right):

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.h:29:
SESSIONS_GETRECENTLYCLOSED)
style nit: align vertically with the first argument. This nit applies to many
other function declarations in this file, possibly elsewhere, but I won't
pollute the comment space with them.

https://codereview.chromium.org/21022018/diff/24001/chrome/common/extensions/...
File chrome/common/extensions/api/sessions.json (right):

https://codereview.chromium.org/21022018/diff/24001/chrome/common/extensions/...
chrome/common/extensions/api/sessions.json:21: "type": "object",
description here should include that this represents a foreign device ("device
on another machine" or whatever the term people use is).

https://codereview.chromium.org/21022018/diff/24001/chrome/common/extensions/...
chrome/common/extensions/api/sessions.json:23: "info": {"type": "string",
"description": "Represents all information about device."},
A string represents all information about the device? that doesn't make sense.

Wasn't this supposed to be an ID into the devices API?

https://codereview.chromium.org/21022018/diff/24001/chrome/common/extensions/...
chrome/common/extensions/api/sessions.json:24: "sessions": {"type": "array",
"items": {"$ref": "Session"}, "description": "Each session represents a window
and its currently open tabs on said device. The array will comprise of all
window entries currently open on related device."}
A list of open window sessions for the [adjective] device, from most recently to
least recently changed. [something about the number of sessions].

https://codereview.chromium.org/21022018/diff/24001/chrome/common/extensions/...
chrome/common/extensions/api/sessions.json:75: "maximum": 10
make this first parameter an object with argument properties:

session.getDevices({
  maxResults: 6
}, function(devices) {
  ...
});

is forwards compatible, whereas

session.getDevices(6, function(devices) {})

isn't. For example, one thing we might want to implement here is paging, which
would be configurable via other properties of the options object.

https://codereview.chromium.org/21022018/diff/24001/chrome/common/extensions/...
chrome/common/extensions/api/sessions.json:82: "name": "devices", "type":
"array", "items": { "$ref": "Device" }, "description": "The list of $ref:Device
objects for each synced session, sorted in order from device with most recently
modified session to device with least recently modified session, and within each
device by most recent to least recently modified session."
"and within each device..." that documentation should be part of the Device type
documentation.

https://codereview.chromium.org/21022018/diff/24001/chrome/common/extensions/...
chrome/common/extensions/api/sessions.json:97: "description": "The API provided
$ref:windows.Window.sessionId, or $ref:tabs.Tab.sessionId to restore."
"The API provided" sounds awkward.

https://codereview.chromium.org/21022018/diff/24001/chrome/common/extensions/...
chrome/common/extensions/api/sessions.json:107: "description": "The restored
$ref:windows.Window or $ref:tabs.Tab object."
I wonder whether this should actually be an object with a window and a tab,
since if we're dealing in abstract session IDs here (which - we can, clearly)
there isn't a non-hacky way to tell the difference.

In fact, you could just return a Session object.

https://codereview.chromium.org/21022018/diff/24001/chrome/common/extensions/...
File chrome/common/extensions/docs/templates/public/extensions/sessions.html
(right):

https://codereview.chromium.org/21022018/diff/24001/chrome/common/extensions/...
chrome/common/extensions/docs/templates/public/extensions/sessions.html:1:
{{+partials.standard_extensions_api api:apis.sessions}}
something funny happened here.

[Kristen Dwan, 2013-08-12 15:11:30]

main conclusion i think with session sync not enabled - in GetDevices, it should
not return an error, simply return an empty list. however, if there is no
associator / service in the restore functions, i think it should be considered
an error since you should have caught that there were no calls to be made after
your devices returned empty.

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
File chrome/browser/extensions/api/sessions/sessions_api.cc (right):

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:56: }
On 2013/08/06 19:04:09, kalman wrote:
> Rather than sharing a magic variable (kForeignIdSeparator) and having util
> classes (those 3), it would be nicer to encapsulate a foreign ID within a
little
> class, providing ToString() and FromString() methods.

Done.

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:67:
extensions::WindowController** controller) {
On 2013/08/06 19:04:09, kalman wrote:
> Please pull this into a common place and share it with the windows API code:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

Done.

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:118: const
TabRestoreService::Tab& tab, const int* const session_id,
On 2013/08/06 19:04:09, kalman wrote:
> The second const isn't necessary.
> 
> That said I think it should be the job of the caller to decide what the
session
> ID is. Pass it in directly. Callers can pass in tab.id if they want.

Done.

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:234:
scoped_ptr<tabs::Tab> SessionsGetDevicesFunction::CreateTabModel(
On 2013/08/06 19:04:09, kalman wrote:
> what's the difference between this and the other CreateTabModel?

This takes a SessionTab and the other takes a TabRestoreService::Tab which don't
share a superclass. Made helpers that group similar behavior... should have done
that before and not just modified the old CLs code.. my bad!

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:258:
scoped_ptr<windows::Window> SessionsGetDevicesFunction::CreateWindowModel(
On 2013/08/06 19:04:09, kalman wrote:
> ditto?

SessionWindow VS TabRestoreService::Window.

Changed to helper. Need to dig a little deeper to find values for
top/right/left/bottom so that can be moved to the helper and the values won't be
optionals.

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:313:
SessionsGetDevicesFunction::CreateSessionModel(
On 2013/08/06 19:04:09, kalman wrote:
> ditto?

SessionWindow vs. TabRestoreService::Entry*. put in helper.

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:339:
SetError(kSessionsSyncNotEnabledError);
On 2013/08/06 19:04:09, kalman wrote:
> Not sure about this one. There should be a way for extensions to know whether
> session sync is enabled, but can't be via an error message. That canonicalises
> error messages, which is *not* what we want.
> 
> I.e. as an extension author, how am I supposed to tell? String-compare error
> messages? Not good.
> 
> If only we had documented and strict error codes in APIs - but we don't. It's
a
> shame.
> 
> Here are some options:
> 
> 1 - have another API call, chrome.sessions.canGetDevices(...). Ugh.
> 
> 2 - make devices null if session sync isn't enabled. A bit bug-inducing since
> extensions that go "if (result.devices)" wouldn't be able to tell between an
> empty list and null. Obviously that's a bug in the extension, but it does
imply
> a confusing API.
> 
> 3 - make results an object rather than an array. too much nesting, yuck.
> 
> Of those, (2) is by far the most preferable. Any other ideas?

I can't think of anything that would be clean and allow you to distinguish
between the two in (2). I also don't think it would be a big deal for the
extension -- a message such as "no devices available, do you have Chrome sync
set up?" or what not.. would be acceptable. Either way, there is no information
available for the extension to display so it doesn't make too much of a
difference to them for what reason it is. The user could figure out between the
two.

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:342: unsigned int
max_results = kMaxSyncedSessionResults;
On 2013/08/06 19:04:09, kalman wrote:
> just "int".

Done.

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:344:
GetDevices::Params::Create(*args_));
On 2013/08/06 19:04:09, kalman wrote:
> 1 line?

Done.

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:359:
SetError(kSyncedSessionsListEmptyError);
On 2013/08/06 19:04:09, kalman wrote:
> This isn't really an *error* case. Similar as before.
> 
> Speaking of which, all of these corner-ish cases, we should document in some
> way.

would you recommend just returning true here then w/ null results?

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:385:
SetError(kRecentlyClosedListEmptyError);
On 2013/08/06 19:04:09, kalman wrote:
> also not an error - and this function shouldn't be returning false (so -
> existing bug).
> 
> hm, we should have a CHECK somewhere in the extension system that "return
false"
> also sets error_. Anyhow.

this is how the current sessionRestore api implementation is. should it change
to just return null? thoughts?

the reason it seems like an error to me is if you're making a call
chrome.sessions.restore() when there is nothing to restore.. returning null /
empty would tell you that nothing was restored but idk if that is the behavior
we want? you shouldn't be able to successfully call restore when there is
nothing to be restored.

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:389: bool is_window =
entries.front()->type == TabRestoreService::WINDOW;
On 2013/08/06 19:04:09, kalman wrote:
> inline this?

Done.

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:393:
SetError(kReturnObjectError);
On 2013/08/06 19:04:09, kalman wrote:
> I think we can do a better job of an error message here.

got rid of this. backend calls are always returning webcontents, so it should
never be the case that it's empty. put in dcheck.

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:398: WindowController*
controller;
On 2013/08/06 19:04:09, kalman wrote:
> always initialise pointers to NULL in situations like these.

Done.

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:402: return false;
On 2013/08/06 19:04:09, kalman wrote:
> error message

Done.

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:415:
browser->tab_strip_model()->GetActiveWebContents());
On 2013/08/06 19:04:09, kalman wrote:
> delegate isn't used until way later, grab it then.
> 
> and is it really the case that it can't be NULL?

"|delegate| will be NULL in cases where one isn't already available (eg, when
invoked on Mac OS X with no windows open)" from the deepest function call in the
stack that this gets passed to.. once again, my bad - shouldn't have relied on
the previous code. sorry :/

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:427: bool is_window =
false;
On 2013/08/06 19:04:09, kalman wrote:
> rather than hold onto these two values, perhaps find a pointer to the entry.
> checks for is_valid_id would be replaced by entry != NULL. CHeck for is_window
> would be directly inspecting the type.

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ses...

when it's a window, the entry gets deleted :(

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:461:
SetError(kReturnObjectError);
On 2013/08/06 19:04:09, kalman wrote:
> yeah so kReturnObject error - not a great error in general

got rid of it.

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:480: const std::string&
session_tag, int id, Browser* browser) {
On 2013/08/06 19:04:09, kalman wrote:
> similar comments throughout here and the next function

Done.

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:484:
SetError(kSessionsSyncNotEnabledError);
Would you agree that these sync errors in restore should stay? I can understand
it not being an error in GetDevices but if you're trying to restore and
something goes wrong I'd assume that would be an error..

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:504: } else {
On 2013/08/06 19:04:09, kalman wrote:
> no else after return

Done.

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
File chrome/browser/extensions/api/sessions/sessions_api.h (right):

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.h:29:
SESSIONS_GETRECENTLYCLOSED)
On 2013/08/06 19:04:09, kalman wrote:
> style nit: align vertically with the first argument. This nit applies to many
> other function declarations in this file, possibly elsewhere, but I won't
> pollute the comment space with them.

Done.

https://codereview.chromium.org/21022018/diff/24001/chrome/common/extensions/...
File chrome/common/extensions/api/sessions.json (right):

https://codereview.chromium.org/21022018/diff/24001/chrome/common/extensions/...
chrome/common/extensions/api/sessions.json:21: "type": "object",
On 2013/08/06 19:04:09, kalman wrote:
> description here should include that this represents a foreign device ("device
> on another machine" or whatever the term people use is).

Done.

https://codereview.chromium.org/21022018/diff/24001/chrome/common/extensions/...
chrome/common/extensions/api/sessions.json:23: "info": {"type": "string",
"description": "Represents all information about device."},
On 2013/08/06 19:04:09, kalman wrote:
> A string represents all information about the device? that doesn't make sense.
> 
> Wasn't this supposed to be an ID into the devices API?

Yeah (this is just a temp placeholder) - it will be the linked
signedInDevices.DeviceInfo. I put a note up in the description. should have
included it in the pub & mail too. Was waiting on lingesh to get the code
checked in for that. Will mail out the addition of DeviceInfo information after
I finish this up.

https://codereview.chromium.org/21022018/diff/24001/chrome/common/extensions/...
chrome/common/extensions/api/sessions.json:24: "sessions": {"type": "array",
"items": {"$ref": "Session"}, "description": "Each session represents a window
and its currently open tabs on said device. The array will comprise of all
window entries currently open on related device."}
On 2013/08/06 19:04:09, kalman wrote:
> A list of open window sessions for the [adjective] device, from most recently
to
> least recently changed. [something about the number of sessions].

Done.

https://codereview.chromium.org/21022018/diff/24001/chrome/common/extensions/...
chrome/common/extensions/api/sessions.json:75: "maximum": 10
On 2013/08/06 19:04:09, kalman wrote:
> make this first parameter an object with argument properties:
> 
> session.getDevices({
>   maxResults: 6
> }, function(devices) {
>   ...
> });
> 
> is forwards compatible, whereas
> 
> session.getDevices(6, function(devices) {})
> 
> isn't. For example, one thing we might want to implement here is paging, which
> would be configurable via other properties of the options object.

Done.

https://codereview.chromium.org/21022018/diff/24001/chrome/common/extensions/...
chrome/common/extensions/api/sessions.json:82: "name": "devices", "type":
"array", "items": { "$ref": "Device" }, "description": "The list of $ref:Device
objects for each synced session, sorted in order from device with most recently
modified session to device with least recently modified session, and within each
device by most recent to least recently modified session."
On 2013/08/06 19:04:09, kalman wrote:
> "and within each device..." that documentation should be part of the Device
type
> documentation.

Done.

https://codereview.chromium.org/21022018/diff/24001/chrome/common/extensions/...
chrome/common/extensions/api/sessions.json:97: "description": "The API provided
$ref:windows.Window.sessionId, or $ref:tabs.Tab.sessionId to restore."
On 2013/08/06 19:04:09, kalman wrote:
> "The API provided" sounds awkward.

Done.

https://codereview.chromium.org/21022018/diff/24001/chrome/common/extensions/...
chrome/common/extensions/api/sessions.json:107: "description": "The restored
$ref:windows.Window or $ref:tabs.Tab object."
On 2013/08/06 19:04:09, kalman wrote:
> I wonder whether this should actually be an object with a window and a tab,
> since if we're dealing in abstract session IDs here (which - we can, clearly)
> there isn't a non-hacky way to tell the difference.
> 
> In fact, you could just return a Session object.

Done.

https://codereview.chromium.org/21022018/diff/24001/chrome/common/extensions/...
File chrome/common/extensions/docs/templates/public/extensions/sessions.html
(right):

https://codereview.chromium.org/21022018/diff/24001/chrome/common/extensions/...
chrome/common/extensions/docs/templates/public/extensions/sessions.html:1:
{{+partials.standard_extensions_api api:apis.sessions}}
On 2013/08/06 19:04:09, kalman wrote:
> something funny happened here.

not sure what that found as it's "previous" file.. probably because I set the
similarity way lower.
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...

[Kristen Dwan, 2013-08-12 15:14:54]

also - i added windows_helper.h and foreign_session_id.h -- the low similarity
decided to detect those as modifications as well... i tried to increase it but
it didn't help.

[not at google - send to devlin, 2013-08-12 23:51:34]

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
File chrome/browser/extensions/api/sessions/sessions_api.cc (right):

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:339:
SetError(kSessionsSyncNotEnabledError);
On 2013/08/12 15:11:30, Kristen Dwan wrote:
> On 2013/08/06 19:04:09, kalman wrote:
> > Not sure about this one. There should be a way for extensions to know
whether
> > session sync is enabled, but can't be via an error message. That
canonicalises
> > error messages, which is *not* what we want.
> > 
> > I.e. as an extension author, how am I supposed to tell? String-compare error
> > messages? Not good.
> > 
> > If only we had documented and strict error codes in APIs - but we don't.
It's
> a
> > shame.
> > 
> > Here are some options:
> > 
> > 1 - have another API call, chrome.sessions.canGetDevices(...). Ugh.
> > 
> > 2 - make devices null if session sync isn't enabled. A bit bug-inducing
since
> > extensions that go "if (result.devices)" wouldn't be able to tell between an
> > empty list and null. Obviously that's a bug in the extension, but it does
> imply
> > a confusing API.
> > 
> > 3 - make results an object rather than an array. too much nesting, yuck.
> > 
> > Of those, (2) is by far the most preferable. Any other ideas?
> 
> I can't think of anything that would be clean and allow you to distinguish
> between the two in (2). I also don't think it would be a big deal for the
> extension -- a message such as "no devices available, do you have Chrome sync
> set up?" or what not.. would be acceptable. Either way, there is no
information
> available for the extension to display so it doesn't make too much of a
> difference to them for what reason it is. The user could figure out between
the
> two.

This sounds good to me.

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:359:
SetError(kSyncedSessionsListEmptyError);
On 2013/08/12 15:11:30, Kristen Dwan wrote:
> On 2013/08/06 19:04:09, kalman wrote:
> > This isn't really an *error* case. Similar as before.
> > 
> > Speaking of which, all of these corner-ish cases, we should document in some
> > way.
> 
> would you recommend just returning true here then w/ null results?

empty results seems consistent.

https://codereview.chromium.org/21022018/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:385:
SetError(kRecentlyClosedListEmptyError);
On 2013/08/12 15:11:30, Kristen Dwan wrote:
> On 2013/08/06 19:04:09, kalman wrote:
> > also not an error - and this function shouldn't be returning false (so -
> > existing bug).
> > 
> > hm, we should have a CHECK somewhere in the extension system that "return
> false"
> > also sets error_. Anyhow.
> 
> this is how the current sessionRestore api implementation is. should it change
> to just return null? thoughts?
> 
> the reason it seems like an error to me is if you're making a call
> chrome.sessions.restore() when there is nothing to restore.. returning null /
> empty would tell you that nothing was restored but idk if that is the behavior
> we want? you shouldn't be able to successfully call restore when there is
> nothing to be restored.

Yeah ok I might have gotten carried away with making this not errors. This
should probably be an error.

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
File chrome/browser/extensions/api/sessions/foreign_session_id.cc (right):

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/foreign_session_id.cc:31: return
(session_tag_ + kIdSeparator + base::IntToString(id_));
use base::StringPrintf instead

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
File chrome/browser/extensions/api/sessions/foreign_session_id.h (right):

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/foreign_session_id.h:17:
ForeignSessionId(const std::string& session_tag, int id);
maybe you need to explain the difference between the tag and the ID.

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/foreign_session_id.h:20: explicit
ForeignSessionId(const std::string& foreign_session_string);
generally nicer to have a static method like Parse or FromString which returns a
ForeignSessionId. Avoid doing initialization work from with a constructor.

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/foreign_session_id.h:28: std::string
session_tag() const { return session_tag_; }
return a const std::string& here

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
File chrome/browser/extensions/api/sessions/sessions_api.cc (right):

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:43: const char
kRecentlyClosedListEmptyError[] =
this name is bugging me. kNoRecentlyClosedSessionsError.

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:45: const char
kInvalidSessionIdError[] = "Invalid session id.";
would be nicer if it included the session ID in here.

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:49: const char
kSyncedSessionsListEmptyError[] = "There are no foreign sessions.";
not used

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:50: const char
kSessionNotFoundError[] = "Session is not found.";
would be nicer if it included the session ID in here.

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:60:
scoped_ptr<extensions::api::tabs::Tab> CreateTabModelHelper(
Put this whole file in the extensions namespace. Then you don't need to qualify
any of these.

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:75:
tab_struct->title.reset(new std::string(title.empty() ? gurl.spec() : title));
This line looked suspicious so I did some digging.

The way that the normal Tab structure gets populated is via
WebContents::GetTitle, which does a bunch of stuff, including - if the title is
empty - delegating to the current NavigationEntry's "display URL", which in turn
calls net::FormatUrl.

We should do the same. See here:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/aut...

Though you can just use the shorter version of that which takes only the URL and
accept-languages.

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:87:
std::vector<linked_ptr<extensions::api::tabs::Tab> >* tabs,
since you're passing ownership to this method, tabs should be a scoped_ptr

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:106:
extensions::api::windows::Window* window) {
pass scoped_ptrs in here.

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:113:
session_struct->window.reset(window);
else
  NOTREACHED()

?

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:117: inline bool
is_type_window(const TabRestoreService::Entry* entry) {
inline doesn't mean anything in this context, compiler can figure it out.

also "is_type_window" looks too vague reading code, is_window_entry perhaps.

in any case, use this everywhere or nowhere. It's used only a couple of times
and there are many other instances of comparing directly to either a TAB or
WINDOW (and if you use everywhere - define an is_tab_entry macro too).

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:164: if (entry->type ==
TabRestoreService::TAB) {
I preferred the switch...

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:167: .release());
CreateTabModel already returns a scoped_ptr so you don't need the release().
Just assign.

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:228:
ForeignSessionId(session_tag, tab.tab_id.id()).ToString();
palindromic expression, nice.

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:234:
GetExtension()).Pass();
I don't think you need to Pass() if the scoped_ptr is already an rvalue? But I'm
just as likely to be wrong.

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:260: default:
default is unnecessary. Just initialize type to TYPE_NONE. let compiler catch
errors.

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:279: state =
windows::Window::STATE_NONE;
ditto

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:284: .release());
release also not needed here

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:415:
SetError(kInvalidSessionIdError);
Doesn't seem like the right error string.

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:447:
SetError(kInvalidSessionIdError);
doesn't seem like the right error string either.

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:458:
DCHECK(contents.size());
I see. Yeah maybe we should just rely on this check and not bother at all about
iterating through the entries apart from finding is_window. Might as well let
the library do this for us - unless for some reason it's not doing it for us.

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:477:
SetError(kSessionSyncNotEnabledError);
let's just make all of the not-found errors the same, regardless of the cause.

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:483:
SetError(kSessionSyncError);
what does it actually mean for this to be null?

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:540: if
(ForeignSessionId::IsForeignSessionId(*params->session_id)) {
it would be ideal if this foreign vs local stuff were all contained in this
class. you can then check like IsForeign, IsLocal, whatever.

and if you had ForeignSessionId::Parse instead of this (returning a scoped_ptr)
then you'd just null check rather than having that extra isForeignSessionId
method.

That's one advantage of a Parse method, lets you actually signal errors.

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
File chrome/browser/extensions/api/tabs/windows_helper.h (right):

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/tabs/windows_helper.h:13: 
file should be in a windows_helper namespace.

Incidentally it's funny when people add helper/util classes, there seems to be a
50/50 chance they either decide to call it "util" or "helper". Or "utils", or
"helpers".

FYI just "util" is the more common in Chrome.

https://codereview.chromium.org/21022018/diff/43001/chrome/common/extensions/...
File chrome/common/extensions/api/sessions.json (right):

https://codereview.chromium.org/21022018/diff/43001/chrome/common/extensions/...
chrome/common/extensions/api/sessions.json:24: "sessions": {"type": "array",
"items": {"$ref": "Session"}, "description": "A list of open window sessions for
the foreign device, sorted in order from most recently to least recently
modified session. The number of sessions is equivalent to the number of windows
open on the foreign device."}
"sorted in order" is a tautology.

By "something about the number" I meant like, how many sessions can there be,
etc.

https://codereview.chromium.org/21022018/diff/43001/chrome/common/extensions/...
chrome/common/extensions/api/sessions.json:80: "description": "The maximum
number of foreign devices to be fetched in the requested list. Omit to fetch the
maximum number of devices (10)."
oh right. This comment confused me and it's because the API confuses me.

What does maximum mean here? Does it mean the maximum number of devices? The
maximum number of sessions per device? The total maximum number of sessions?

I presume it's the second of those, but the comment needs to make that clear.

And we need to make sure the implementation does that.

[Kristen Dwan, 2013-08-15 07:11:56]

addressed all. unfortunately, renaming session_id class file caused a loss in
diff from foreign_session_id.. cr decided that another random file matched up
with them.. 

sill need to decide if 4 different errors is too many for the api. I combined
them down to that.

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
File chrome/browser/extensions/api/sessions/foreign_session_id.cc (right):

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/foreign_session_id.cc:31: return
(session_tag_ + kIdSeparator + base::IntToString(id_));
On 2013/08/12 23:51:34, kalman wrote:
> use base::StringPrintf instead

Done.

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
File chrome/browser/extensions/api/sessions/foreign_session_id.h (right):

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/foreign_session_id.h:17:
ForeignSessionId(const std::string& session_tag, int id);
On 2013/08/12 23:51:34, kalman wrote:
> maybe you need to explain the difference between the tag and the ID.

Done.

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/foreign_session_id.h:20: explicit
ForeignSessionId(const std::string& foreign_session_string);
On 2013/08/12 23:51:34, kalman wrote:
> generally nicer to have a static method like Parse or FromString which returns
a
> ForeignSessionId. Avoid doing initialization work from with a constructor.

Done.

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/foreign_session_id.h:28: std::string
session_tag() const { return session_tag_; }
On 2013/08/12 23:51:34, kalman wrote:
> return a const std::string& here

Done.

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
File chrome/browser/extensions/api/sessions/sessions_api.cc (right):

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:43: const char
kRecentlyClosedListEmptyError[] =
On 2013/08/12 23:51:34, kalman wrote:
> this name is bugging me. kNoRecentlyClosedSessionsError.

Done.

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:45: const char
kInvalidSessionIdError[] = "Invalid session id.";
On 2013/08/12 23:51:34, kalman wrote:
> would be nicer if it included the session ID in here.

Done.

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:49: const char
kSyncedSessionsListEmptyError[] = "There are no foreign sessions.";
On 2013/08/12 23:51:34, kalman wrote:
> not used

Done.

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:50: const char
kSessionNotFoundError[] = "Session is not found.";
On 2013/08/12 23:51:34, kalman wrote:
> would be nicer if it included the session ID in here.

no reason to have session not found and invalid session errors. obviously if
it's not found .. it's invalid. so i put them into one.

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:60:
scoped_ptr<extensions::api::tabs::Tab> CreateTabModelHelper(
On 2013/08/12 23:51:34, kalman wrote:
> Put this whole file in the extensions namespace. Then you don't need to
qualify
> any of these.

Done.

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:75:
tab_struct->title.reset(new std::string(title.empty() ? gurl.spec() : title));
On 2013/08/12 23:51:34, kalman wrote:
> This line looked suspicious so I did some digging.
> 
> The way that the normal Tab structure gets populated is via
> WebContents::GetTitle, which does a bunch of stuff, including - if the title
is
> empty - delegating to the current NavigationEntry's "display URL", which in
turn
> calls net::FormatUrl.
> 
> We should do the same. See here:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/aut...
> 
> Though you can just use the shorter version of that which takes only the URL
and
> accept-languages.

Done.

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:87:
std::vector<linked_ptr<extensions::api::tabs::Tab> >* tabs,
On 2013/08/12 23:51:34, kalman wrote:
> since you're passing ownership to this method, tabs should be a scoped_ptr

Done.

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:106:
extensions::api::windows::Window* window) {
On 2013/08/12 23:51:34, kalman wrote:
> pass scoped_ptrs in here.

Done.

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:113:
session_struct->window.reset(window);
On 2013/08/12 23:51:34, kalman wrote:
> else
>   NOTREACHED()
> 
> ?

Done.

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:117: inline bool
is_type_window(const TabRestoreService::Entry* entry) {
On 2013/08/12 23:51:34, kalman wrote:
> inline doesn't mean anything in this context, compiler can figure it out.
> 
> also "is_type_window" looks too vague reading code, is_window_entry perhaps.
> 
> in any case, use this everywhere or nowhere. It's used only a couple of times
> and there are many other instances of comparing directly to either a TAB or
> WINDOW (and if you use everywhere - define an is_tab_entry macro too).

Done.

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:164: if (entry->type ==
TabRestoreService::TAB) {
On 2013/08/12 23:51:34, kalman wrote:
> I preferred the switch...

Done.

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:167: .release());
On 2013/08/12 23:51:34, kalman wrote:
> CreateTabModel already returns a scoped_ptr so you don't need the release().
> Just assign.

Done.

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:234:
GetExtension()).Pass();
On 2013/08/12 23:51:34, kalman wrote:
> I don't think you need to Pass() if the scoped_ptr is already an rvalue? But
I'm
> just as likely to be wrong.

Done.

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:260: default:
On 2013/08/12 23:51:34, kalman wrote:
> default is unnecessary. Just initialize type to TYPE_NONE. let compiler catch
> errors.

Done.

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:279: state =
windows::Window::STATE_NONE;
On 2013/08/12 23:51:34, kalman wrote:
> ditto

Done.

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:284: .release());
On 2013/08/12 23:51:34, kalman wrote:
> release also not needed here

Done.

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:415:
SetError(kInvalidSessionIdError);
On 2013/08/12 23:51:34, kalman wrote:
> Doesn't seem like the right error string.

i figured if you passed in an id and sessions are empty .. you obviously passed
an invalid id. but i could make it kNoRecentlyClosedSessionsError as well. but
it seemed more fitting to say - look, this id is invalid.

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:447:
SetError(kInvalidSessionIdError);
On 2013/08/12 23:51:34, kalman wrote:
> doesn't seem like the right error string either.

if you didn't find the specific id, it would be invalid..?

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:458:
DCHECK(contents.size());
On 2013/08/12 23:51:34, kalman wrote:
> I see. Yeah maybe we should just rely on this check and not bother at all
about
> iterating through the entries apart from finding is_window. Might as well let
> the library do this for us - unless for some reason it's not doing it for us.

good point. it does the exact thing. but before there was no way of knowing what
happened since the call to restore was void. updated to only check for if it's a
window.

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:477:
SetError(kSessionSyncNotEnabledError);
On 2013/08/12 23:51:34, kalman wrote:
> let's just make all of the not-found errors the same, regardless of the cause.

we can't include the session id in the error message then if combine all, since
it's an optional parameter. in any case i combined sync availability errors into
one. down to 4 different errors in total..

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:483:
SetError(kSessionSyncError);
On 2013/08/12 23:51:34, kalman wrote:
> what does it actually mean for this to be null?

"Returns the session model associator associated with this type -- (in this case
the type is syncer::SESSIONS) --, but only if the associator is running.  If it
is doing anything else, it will return null."

soo, basically it's null if there exists no data type controller associated with
the model or if the controller isn't in "running" state -- meaning in sync with
the cloud. and if it's null, we have no way of accessing the synced information.

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:540: if
(ForeignSessionId::IsForeignSessionId(*params->session_id)) {
On 2013/08/12 23:51:34, kalman wrote:
> it would be ideal if this foreign vs local stuff were all contained in this
> class. you can then check like IsForeign, IsLocal, whatever.
> 
> and if you had ForeignSessionId::Parse instead of this (returning a
scoped_ptr)
> then you'd just null check rather than having that extra isForeignSessionId
> method.
> 
> That's one advantage of a Parse method, lets you actually signal errors.

way nicer now :)

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
File chrome/browser/extensions/api/tabs/windows_helper.h (right):

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/tabs/windows_helper.h:13: 
On 2013/08/12 23:51:34, kalman wrote:
> file should be in a windows_helper namespace.
> 
> Incidentally it's funny when people add helper/util classes, there seems to be
a
> 50/50 chance they either decide to call it "util" or "helper". Or "utils", or
> "helpers".
> 
> FYI just "util" is the more common in Chrome.

just based it off the most recent file i saw added, id_mapping_helper in
signedin_devices api, hah. i like utils though, we'll go with that.

https://codereview.chromium.org/21022018/diff/43001/chrome/common/extensions/...
File chrome/common/extensions/api/sessions.json (right):

https://codereview.chromium.org/21022018/diff/43001/chrome/common/extensions/...
chrome/common/extensions/api/sessions.json:24: "sessions": {"type": "array",
"items": {"$ref": "Session"}, "description": "A list of open window sessions for
the foreign device, sorted in order from most recently to least recently
modified session. The number of sessions is equivalent to the number of windows
open on the foreign device."}
On 2013/08/12 23:51:34, kalman wrote:
> "sorted in order" is a tautology.
> 
> By "something about the number" I meant like, how many sessions can there be,
> etc.

so ... there is no limit on the number of sessions returned. we should probably
set one. i haven't seen any sort of cap in chrome backend - other than sessions
become stale after 14 days.

https://codereview.chromium.org/21022018/diff/43001/chrome/common/extensions/...
chrome/common/extensions/api/sessions.json:80: "description": "The maximum
number of foreign devices to be fetched in the requested list. Omit to fetch the
maximum number of devices (10)."
On 2013/08/12 23:51:34, kalman wrote:
> oh right. This comment confused me and it's because the API confuses me.
> 
> What does maximum mean here? Does it mean the maximum number of devices? The
> maximum number of sessions per device? The total maximum number of sessions?
> 
> I presume it's the second of those, but the comment needs to make that clear.
> 
> And we need to make sure the implementation does that.

nope, it's the maximum number of devices that can be returned -- as in the max
possible length of the Device[] devices. which .... basically no one has 10
synced devices.

but it gets weird since in the backend a "session" is what we call a
"sessions.Device" .. and each "sessions.Device.sessions" is a single window or
tab that is contained in the backend "session".. basically in the backend
session means full device but in the exposure of the api the term session means
a single "browsing session" as in a single window or tab.

[not at google - send to devlin, 2013-08-15 20:16:23]

The only remaining major comment here is the thing about limiting with that
filtering object.

Other than that lg.

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
File chrome/browser/extensions/api/sessions/sessions_api.cc (right):

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:415:
SetError(kInvalidSessionIdError);
On 2013/08/15 07:11:56, Kristen Dwan wrote:
> On 2013/08/12 23:51:34, kalman wrote:
> > Doesn't seem like the right error string.
> 
> i figured if you passed in an id and sessions are empty .. you obviously
passed
> an invalid id. but i could make it kNoRecentlyClosedSessionsError as well. but
> it seemed more fitting to say - look, this id is invalid.

Makes sense.

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:447:
SetError(kInvalidSessionIdError);
On 2013/08/15 07:11:56, Kristen Dwan wrote:
> On 2013/08/12 23:51:34, kalman wrote:
> > doesn't seem like the right error string either.
> 
> if you didn't find the specific id, it would be invalid..?

Makes sense, sorry.

https://codereview.chromium.org/21022018/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:477:
SetError(kSessionSyncNotEnabledError);
On 2013/08/15 07:11:56, Kristen Dwan wrote:
> On 2013/08/12 23:51:34, kalman wrote:
> > let's just make all of the not-found errors the same, regardless of the
cause.
> 
> we can't include the session id in the error message then if combine all,
since
> it's an optional parameter. in any case i combined sync availability errors
into
> one. down to 4 different errors in total..

Ah makes sense.

https://codereview.chromium.org/21022018/diff/43001/chrome/common/extensions/...
File chrome/common/extensions/api/sessions.json (right):

https://codereview.chromium.org/21022018/diff/43001/chrome/common/extensions/...
chrome/common/extensions/api/sessions.json:80: "description": "The maximum
number of foreign devices to be fetched in the requested list. Omit to fetch the
maximum number of devices (10)."
On 2013/08/15 07:11:56, Kristen Dwan wrote:
> On 2013/08/12 23:51:34, kalman wrote:
> > oh right. This comment confused me and it's because the API confuses me.
> > 
> > What does maximum mean here? Does it mean the maximum number of devices? The
> > maximum number of sessions per device? The total maximum number of sessions?
> > 
> > I presume it's the second of those, but the comment needs to make that
clear.
> > 
> > And we need to make sure the implementation does that.
> 
> nope, it's the maximum number of devices that can be returned -- as in the max
> possible length of the Device[] devices. which .... basically no one has 10
> synced devices.
> 
> but it gets weird since in the backend a "session" is what we call a
> "sessions.Device" .. and each "sessions.Device.sessions" is a single window or
> tab that is contained in the backend "session".. basically in the backend
> session means full device but in the exposure of the api the term session
means
> a single "browsing session" as in a single window or tab.

Ok. Having a limit on the number of devices doesn't make sense since it's a
finite amount of data (even if chances are nobody has 10 devices).

The history of sessions isn't finite though; that's what I'd expect us to be
limiting. 14 days worth of history is going to be pretty pretty enormous, so,
I'd like to add something for that - in the same way that getRecentlyClosed
does.

In fact I think we may even want to define a general "filter" object and use
that for both here and getRecentlyClosed. Maximum results (defaulting to 25,
sure), type of session (for foreign presumably it'll just always be window?), in
the future if people request it we could also have a time limit rather than a
volume limit.

https://codereview.chromium.org/21022018/diff/87002/chrome/browser/extensions...
File chrome/browser/extensions/api/sessions/session_id.cc (right):

https://codereview.chromium.org/21022018/diff/87002/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/session_id.cc:36: return
scoped_ptr<SessionId>(new SessionId(session_tag, id));
you can use make_scoped_ptr(new SessionId(session_tag, id)) here

https://codereview.chromium.org/21022018/diff/87002/chrome/browser/extensions...
File chrome/browser/extensions/api/sessions/session_id.h (right):

https://codereview.chromium.org/21022018/diff/87002/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/session_id.h:17: // Constructs a
SessionId object for the given session information.
wow. git. weird. damn.

can worry about fixing this diff before submitting I suppose.

https://codereview.chromium.org/21022018/diff/87002/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/session_id.h:27: static
scoped_ptr<SessionId> Parse(const std::string& session_string);
put static methods first. You should also mention that it accepts the format
that ToString would produce and vice versa.

https://codereview.chromium.org/21022018/diff/87002/chrome/browser/extensions...
File chrome/browser/extensions/api/sessions/sessions_api.cc (right):

https://codereview.chromium.org/21022018/diff/87002/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:50: const int
kMaxDeviceResults = 10;
See comment in last patchset; I don't think having a max number of devices is
very useful, but limiting the number of sessions per devices would be.

https://codereview.chromium.org/21022018/diff/87002/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:83: const std::string
languages(
general nit: prefer assign syntax = over constructor syntax for situations like
these.

https://codereview.chromium.org/21022018/diff/87002/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:87:
net::UnescapeRule::NORMAL, NULL, NULL, NULL))));
I think you only need the first 2 arguments.

https://codereview.chromium.org/21022018/diff/87002/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:105:
window_struct->tabs.reset(tabs.release());
should be able to do window_struct->tabs = tabs.Pass() here.

ditto the few places below here.

https://codereview.chromium.org/21022018/diff/87002/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:288: break;
 but no need for the default: case.

https://codereview.chromium.org/21022018/diff/87002/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:372: results_ =
Restore::Results::Create(*CreateSessionModelHelper(
I think this is dereferencing a just-released pointer, if I'm organising the
parentheses correctly. *CreateSessionModelHelper(...).release(). You'll need to
save a reference to the scoped_ptr beforehand.

https://codereview.chromium.org/21022018/diff/87002/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:421: bool
SessionsRestoreFunction::RestoreLocalSession(const SessionId* session_id,
just pass in a const ref

https://codereview.chromium.org/21022018/diff/87002/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:469: bool
SessionsRestoreFunction::RestoreForeignSession(const SessionId* session_id,
const ref

https://codereview.chromium.org/21022018/diff/87002/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:507: &&
(*window)->window_id.id() != session_id->id()) {
nitty align condition vertically

https://codereview.chromium.org/21022018/diff/87002/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:549: return
RestoreLocalSession(session_id.get(), browser);
ternary if you feel like it

https://codereview.chromium.org/21022018/diff/87002/chrome/browser/extensions...
File chrome/browser/extensions/api/tabs/windows_util.cc (right):

https://codereview.chromium.org/21022018/diff/87002/chrome/browser/extensions...
chrome/browser/extensions/api/tabs/windows_util.cc:16: 
comment here would be nice

https://codereview.chromium.org/21022018/diff/87002/chrome/browser/extensions...
File chrome/browser/extensions/api/tabs/windows_util.h (left):

https://codereview.chromium.org/21022018/diff/87002/chrome/browser/extensions...
chrome/browser/extensions/api/tabs/windows_util.h:15: class
MediaStreamRegistryInterface {
git being silly again.

[dharcourt, 2013-08-16 02:27:33]

I have a few grammar/style nits after reading the API documentation generated
from this code. See below.

Overall, the API is beautiful now, people reading it will think "duh, of course,
can there be any other way to do this?" :-). Much better than the first pass and
the API it replaces, thanks for making this work!

    - Charles

https://codereview.chromium.org/21022018/diff/87002/chrome/common/extensions/...
File chrome/common/extensions/api/sessions.json (right):

https://codereview.chromium.org/21022018/diff/87002/chrome/common/extensions/...
chrome/common/extensions/api/sessions.json:44: "description": "The maximum
number of entries to be fetched in the requested list. Omit to fetch the maximum
number of entries (25)."
Nit: "Omit to fetch..." should be "Omit this parameter to fetch...", otherwise
the sentence ambiguous - it could mean "You should not fetch the maximum number
of entries".

I know this came from the sessionRestore API, but this is a good opportunity to
remove the unnecessary ambiguity.

https://codereview.chromium.org/21022018/diff/87002/chrome/common/extensions/...
chrome/common/extensions/api/sessions.json:50: "description": "The type of entry
to fetch. Omit to fetch all types of entries."
Nit: "Omit to fetch" -> "Omit this parameter to fetch", as above.

https://codereview.chromium.org/21022018/diff/87002/chrome/common/extensions/...
chrome/common/extensions/api/sessions.json:59: "name": "sessions", "type":
"array", "items": { "$ref": "Session" }, "description": "The list of closed
entries in reverse order that they were closed (the most recently closed tab or
window will be at index <code>0</code>). If <code>entryType</code> is specified
the entries will be only of that type, otherwise the entries may contain either
tabs or windows.List of $ref:Session representing the closed tab/window for the
session."
Nit: The last sentence (after "either tabs or windows.") is redundant. Also,
there's a space missing after the preceding sentence. Removing the last sentence
will fix both of these issues.

https://codereview.chromium.org/21022018/diff/87002/chrome/common/extensions/...
chrome/common/extensions/api/sessions.json:68: "description": "Retrieves all
synced sessions and returns in sorted order from device with most recently
modified session to device with least recently modified session, and within each
device by most recent to least recently modified session.",
Nit: This was confusing to me. I think the following is more exact and would be
clearer: "Retrieves all devices with synced sessions in sorted order from the
device with most recently modified sessions to the one with least recently
modified sessions. Within each device sessions are sorted from most recently to
least recently modified."

The sort order is actually explained in the description of the callback
parameter and Device type, so this could actually be simplified to the even
clearer: "Retrieves all devices with synced sessions."

https://codereview.chromium.org/21022018/diff/87002/chrome/common/extensions/...
chrome/common/extensions/api/sessions.json:80: "description": "The maximum
number of devices to be returned in the requested $ref:Device list. Omit to
fetch the maximum number of devices (10)."
Nit: As above, "Omit to fetch" -> "Omit this parameter to fetch".

https://codereview.chromium.org/21022018/diff/87002/chrome/common/extensions/...
File chrome/common/extensions/api/tabs.json (right):

https://codereview.chromium.org/21022018/diff/87002/chrome/common/extensions/...
chrome/common/extensions/api/tabs.json:14: "id": {"type": "integer", "minimum":
0, "optional": true, "description": "The ID of the tab. Tab IDs are unique
within a browser session. Under some circumstances a Tab may not be assigned an
ID, for example when querying foreign tabs from the $ref:sessions API, in which
case sessionId may be present."},
Nit: "when querying foreign tabs from" -> "when querying foreign tabs using",
and "sessionId may be present" -> "a session ID may be present" (to match the
"an ID" in the first part of the sentence).

https://codereview.chromium.org/21022018/diff/87002/chrome/common/extensions/...
chrome/common/extensions/api/tabs.json:28: "sessionId": {"type": "string",
"optional": true, "description": "The session ID used to uniquely identify a Tab
from the $ref:sessions API."}
Nit: "a Tab from" -> "a Tab obtained from".

https://codereview.chromium.org/21022018/diff/87002/chrome/common/extensions/...
File chrome/common/extensions/api/windows.json (right):

https://codereview.chromium.org/21022018/diff/87002/chrome/common/extensions/...
chrome/common/extensions/api/windows.json:17: "id": {"type": "integer",
"optional": true, "minimum": 0, "description": "The ID of the window. Window IDs
are unique within a browser session. Under some circumstances a Window may not
be assigned an ID, for example when querying windows from the $ref:sessions API,
in which case sessionId may be present."},
Nit: "when querying windows from" -> "when querying windows using", and
"sessionId may be present" -> "a session ID may be present" (to match the "an
ID" in the first part of the sentence).

https://codereview.chromium.org/21022018/diff/87002/chrome/common/extensions/...
chrome/common/extensions/api/windows.json:38: "sessionId": {"type": "string",
"optional": true, "description": "The session ID used to uniquely identify a
Window from the $ref:sessions API."}
Nit: "a Window from" -> "a Window obtained from".

[Kristen Dwan, 2013-08-16 22:03:05]

Added filter object to sessions.json, not sure about my specification about
always returning windows even if entryType is specified for GetDevices. also,
added sorting to the tabs per window in GetDevices function.

https://codereview.chromium.org/21022018/diff/43001/chrome/common/extensions/...
File chrome/common/extensions/api/sessions.json (right):

https://codereview.chromium.org/21022018/diff/43001/chrome/common/extensions/...
chrome/common/extensions/api/sessions.json:80: "description": "The maximum
number of foreign devices to be fetched in the requested list. Omit to fetch the
maximum number of devices (10)."
On 2013/08/15 20:16:24, kalman wrote:
> On 2013/08/15 07:11:56, Kristen Dwan wrote:
> > On 2013/08/12 23:51:34, kalman wrote:
> > > oh right. This comment confused me and it's because the API confuses me.
> > > 
> > > What does maximum mean here? Does it mean the maximum number of devices?
The
> > > maximum number of sessions per device? The total maximum number of
sessions?
> > > 
> > > I presume it's the second of those, but the comment needs to make that
> clear.
> > > 
> > > And we need to make sure the implementation does that.
> > 
> > nope, it's the maximum number of devices that can be returned -- as in the
max
> > possible length of the Device[] devices. which .... basically no one has 10
> > synced devices.
> > 
> > but it gets weird since in the backend a "session" is what we call a
> > "sessions.Device" .. and each "sessions.Device.sessions" is a single window
or
> > tab that is contained in the backend "session".. basically in the backend
> > session means full device but in the exposure of the api the term session
> means
> > a single "browsing session" as in a single window or tab.
> 
> Ok. Having a limit on the number of devices doesn't make sense since it's a
> finite amount of data (even if chances are nobody has 10 devices).
> 
> The history of sessions isn't finite though; that's what I'd expect us to be
> limiting. 14 days worth of history is going to be pretty pretty enormous, so,
> I'd like to add something for that - in the same way that getRecentlyClosed
> does.
> 
> In fact I think we may even want to define a general "filter" object and use
> that for both here and getRecentlyClosed. Maximum results (defaulting to 25,
> sure), type of session (for foreign presumably it'll just always be window?),
in
> the future if people request it we could also have a time limit rather than a
> volume limit.

it's 14 days but the tabs / windows also have to *currently* be open. so it's
saying they'll be pruned if you leave 80 tabs sitting open for over 14 days,
they'll no longer show up. Closed tabs / windows do not apply and will not be
returned. so it really shouldn't be that enormous... unless you leave tons and
tons of sessions open.

Having a general filter object sounds like a good plan to me. although, how will
we specify that when using the filter for getDevices it will always be window
even if the user specifies tab..

https://codereview.chromium.org/21022018/diff/87002/chrome/browser/extensions...
File chrome/browser/extensions/api/sessions/session_id.cc (right):

https://codereview.chromium.org/21022018/diff/87002/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/session_id.cc:36: return
scoped_ptr<SessionId>(new SessionId(session_tag, id));
On 2013/08/15 20:16:24, kalman wrote:
> you can use make_scoped_ptr(new SessionId(session_tag, id)) here

Done.

https://codereview.chromium.org/21022018/diff/87002/chrome/browser/extensions...
File chrome/browser/extensions/api/sessions/session_id.h (right):

https://codereview.chromium.org/21022018/diff/87002/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/session_id.h:27: static
scoped_ptr<SessionId> Parse(const std::string& session_string);
On 2013/08/15 20:16:24, kalman wrote:
> put static methods first. You should also mention that it accepts the format
> that ToString would produce and vice versa.

Done.

https://codereview.chromium.org/21022018/diff/87002/chrome/browser/extensions...
File chrome/browser/extensions/api/sessions/sessions_api.cc (right):

https://codereview.chromium.org/21022018/diff/87002/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:50: const int
kMaxDeviceResults = 10;
On 2013/08/15 20:16:24, kalman wrote:
> See comment in last patchset; I don't think having a max number of devices is
> very useful, but limiting the number of sessions per devices would be.

Done.

https://codereview.chromium.org/21022018/diff/87002/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:83: const std::string
languages(
On 2013/08/15 20:16:24, kalman wrote:
> general nit: prefer assign syntax = over constructor syntax for situations
like
> these.

Done.

https://codereview.chromium.org/21022018/diff/87002/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:87:
net::UnescapeRule::NORMAL, NULL, NULL, NULL))));
On 2013/08/15 20:16:24, kalman wrote:
> I think you only need the first 2 arguments.

Done.

https://codereview.chromium.org/21022018/diff/87002/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:105:
window_struct->tabs.reset(tabs.release());
On 2013/08/15 20:16:24, kalman wrote:
> should be able to do window_struct->tabs = tabs.Pass() here.
> 
> ditto the few places below here.

Done.

https://codereview.chromium.org/21022018/diff/87002/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:288: break;
On 2013/08/15 20:16:24, kalman wrote:
>  but no need for the default: case.

there are 4 cases not dealt with here that we just consider state::none..
(default, inactive, detached, and end) that are causing compiler errors w/o
default.

https://codereview.chromium.org/21022018/diff/87002/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:372: results_ =
Restore::Results::Create(*CreateSessionModelHelper(
On 2013/08/15 20:16:24, kalman wrote:
> I think this is dereferencing a just-released pointer, if I'm organising the
> parentheses correctly. *CreateSessionModelHelper(...).release(). You'll need
to
> save a reference to the scoped_ptr beforehand.

Done.

https://codereview.chromium.org/21022018/diff/87002/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:421: bool
SessionsRestoreFunction::RestoreLocalSession(const SessionId* session_id,
On 2013/08/15 20:16:24, kalman wrote:
> just pass in a const ref

Done.

https://codereview.chromium.org/21022018/diff/87002/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:469: bool
SessionsRestoreFunction::RestoreForeignSession(const SessionId* session_id,
On 2013/08/15 20:16:24, kalman wrote:
> const ref

Done.

https://codereview.chromium.org/21022018/diff/87002/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:507: &&
(*window)->window_id.id() != session_id->id()) {
On 2013/08/15 20:16:24, kalman wrote:
> nitty align condition vertically

Done.

https://codereview.chromium.org/21022018/diff/87002/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:549: return
RestoreLocalSession(session_id.get(), browser);
On 2013/08/15 20:16:24, kalman wrote:
> ternary if you feel like it

Done.

https://codereview.chromium.org/21022018/diff/87002/chrome/browser/extensions...
File chrome/browser/extensions/api/tabs/windows_util.cc (right):

https://codereview.chromium.org/21022018/diff/87002/chrome/browser/extensions...
chrome/browser/extensions/api/tabs/windows_util.cc:16: 
On 2013/08/15 20:16:24, kalman wrote:
> comment here would be nice

put one in header.

https://codereview.chromium.org/21022018/diff/87002/chrome/common/extensions/...
File chrome/common/extensions/api/sessions.json (right):

https://codereview.chromium.org/21022018/diff/87002/chrome/common/extensions/...
chrome/common/extensions/api/sessions.json:44: "description": "The maximum
number of entries to be fetched in the requested list. Omit to fetch the maximum
number of entries (25)."
On 2013/08/16 02:27:34, dharcourt wrote:
> Nit: "Omit to fetch..." should be "Omit this parameter to fetch...", otherwise
> the sentence ambiguous - it could mean "You should not fetch the maximum
number
> of entries".
> 
> I know this came from the sessionRestore API, but this is a good opportunity
to
> remove the unnecessary ambiguity.

Done.

https://codereview.chromium.org/21022018/diff/87002/chrome/common/extensions/...
chrome/common/extensions/api/sessions.json:50: "description": "The type of entry
to fetch. Omit to fetch all types of entries."
On 2013/08/16 02:27:34, dharcourt wrote:
> Nit: "Omit to fetch" -> "Omit this parameter to fetch", as above.

Done.

https://codereview.chromium.org/21022018/diff/87002/chrome/common/extensions/...
chrome/common/extensions/api/sessions.json:59: "name": "sessions", "type":
"array", "items": { "$ref": "Session" }, "description": "The list of closed
entries in reverse order that they were closed (the most recently closed tab or
window will be at index <code>0</code>). If <code>entryType</code> is specified
the entries will be only of that type, otherwise the entries may contain either
tabs or windows.List of $ref:Session representing the closed tab/window for the
session."
On 2013/08/16 02:27:34, dharcourt wrote:
> Nit: The last sentence (after "either tabs or windows.") is redundant. Also,
> there's a space missing after the preceding sentence. Removing the last
sentence
> will fix both of these issues.

Done.

https://codereview.chromium.org/21022018/diff/87002/chrome/common/extensions/...
chrome/common/extensions/api/sessions.json:68: "description": "Retrieves all
synced sessions and returns in sorted order from device with most recently
modified session to device with least recently modified session, and within each
device by most recent to least recently modified session.",
On 2013/08/16 02:27:34, dharcourt wrote:
> Nit: This was confusing to me. I think the following is more exact and would
be
> clearer: "Retrieves all devices with synced sessions in sorted order from the
> device with most recently modified sessions to the one with least recently
> modified sessions. Within each device sessions are sorted from most recently
to
> least recently modified."
> 
> The sort order is actually explained in the description of the callback
> parameter and Device type, so this could actually be simplified to the even
> clearer: "Retrieves all devices with synced sessions."

Done.

https://codereview.chromium.org/21022018/diff/87002/chrome/common/extensions/...
chrome/common/extensions/api/sessions.json:80: "description": "The maximum
number of devices to be returned in the requested $ref:Device list. Omit to
fetch the maximum number of devices (10)."
On 2013/08/16 02:27:34, dharcourt wrote:
> Nit: As above, "Omit to fetch" -> "Omit this parameter to fetch".

Done.

https://codereview.chromium.org/21022018/diff/87002/chrome/common/extensions/...
File chrome/common/extensions/api/tabs.json (right):

https://codereview.chromium.org/21022018/diff/87002/chrome/common/extensions/...
chrome/common/extensions/api/tabs.json:14: "id": {"type": "integer", "minimum":
0, "optional": true, "description": "The ID of the tab. Tab IDs are unique
within a browser session. Under some circumstances a Tab may not be assigned an
ID, for example when querying foreign tabs from the $ref:sessions API, in which
case sessionId may be present."},
On 2013/08/16 02:27:34, dharcourt wrote:
> Nit: "when querying foreign tabs from" -> "when querying foreign tabs using",
> and "sessionId may be present" -> "a session ID may be present" (to match the
> "an ID" in the first part of the sentence).

Done.

https://codereview.chromium.org/21022018/diff/87002/chrome/common/extensions/...
chrome/common/extensions/api/tabs.json:28: "sessionId": {"type": "string",
"optional": true, "description": "The session ID used to uniquely identify a Tab
from the $ref:sessions API."}
On 2013/08/16 02:27:34, dharcourt wrote:
> Nit: "a Tab from" -> "a Tab obtained from".

Done.

https://codereview.chromium.org/21022018/diff/87002/chrome/common/extensions/...
File chrome/common/extensions/api/windows.json (right):

https://codereview.chromium.org/21022018/diff/87002/chrome/common/extensions/...
chrome/common/extensions/api/windows.json:17: "id": {"type": "integer",
"optional": true, "minimum": 0, "description": "The ID of the window. Window IDs
are unique within a browser session. Under some circumstances a Window may not
be assigned an ID, for example when querying windows from the $ref:sessions API,
in which case sessionId may be present."},
On 2013/08/16 02:27:34, dharcourt wrote:
> Nit: "when querying windows from" -> "when querying windows using", and
> "sessionId may be present" -> "a session ID may be present" (to match the "an
> ID" in the first part of the sentence).

Done.

https://codereview.chromium.org/21022018/diff/87002/chrome/common/extensions/...
chrome/common/extensions/api/windows.json:38: "sessionId": {"type": "string",
"optional": true, "description": "The session ID used to uniquely identify a
Window from the $ref:sessions API."}
On 2013/08/16 02:27:34, dharcourt wrote:
> Nit: "a Window from" -> "a Window obtained from".

Done.

[dharcourt, 2013-08-19 17:53:12]

https://codereview.chromium.org/21022018/diff/114001/chrome/common/extensions...
File chrome/common/extensions/api/sessions.json (right):

https://codereview.chromium.org/21022018/diff/114001/chrome/common/extensions...
chrome/common/extensions/api/sessions.json:28: },
FWIW, there are trailing spaces on this line (28) and this may (should?) cause
trybots to fail.

https://codereview.chromium.org/21022018/diff/114001/chrome/common/extensions...
chrome/common/extensions/api/sessions.json:55: "name": "options",
Nit: "options" -> "filter"?

https://codereview.chromium.org/21022018/diff/114001/chrome/common/extensions...
chrome/common/extensions/api/sessions.json:76: "name": "options",
Nit: "options" -> "filter"?

[not at google - send to devlin, 2013-08-19 19:34:39]

lgtm, question about the documentation for specifying a tabs filter answered.

Speaking of which it would be good to test that. And speaking of that - I
haven't really looked at the tests but I find them a little hard to read, do you
have a plan for them?

If there is an issue with specifying the constant in the json then ping me.

Also: there is now an OWNERS file in api/session_restore so could you move that
over as well.

https://codereview.chromium.org/21022018/diff/87002/chrome/browser/extensions...
File chrome/browser/extensions/api/sessions/sessions_api.cc (right):

https://codereview.chromium.org/21022018/diff/87002/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:49: const int
kMaxRecentlyClosedSessionResults = 25;
I just remembered something else. You can define constants in the schema files,
which is great because (a) it actually gets documented, (b) the documentation
will always be up to date with reality, and (c) the extensions can access that
data as well (e.g. via chrome.sessions.MAX_RECENTLY_CLOSED_SESSION_RESULTS.

OTOH you really need to make sure it has a good name.

Example:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...

and use:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

P.S. hoping 25 isn't too few. Let's see.

https://codereview.chromium.org/21022018/diff/87002/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:288: break;
On 2013/08/16 22:03:06, Kristen Dwan wrote:
> On 2013/08/15 20:16:24, kalman wrote:
> >  but no need for the default: case.
> 
> there are 4 cases not dealt with here that we just consider state::none..
> (default, inactive, detached, and end) that are causing compiler errors w/o
> default.

That's ok, just be explicit about those 4 arguments. Fall-through cases etc.

https://codereview.chromium.org/21022018/diff/87002/chrome/browser/extensions...
File chrome/browser/extensions/api/sessions/sessions_api.h (right):

https://codereview.chromium.org/21022018/diff/87002/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.h:26: struct SessionId;
forward declarations should still work for references?

https://codereview.chromium.org/21022018/diff/114001/chrome/browser/extension...
File chrome/browser/extensions/api/sessions/sessions_api.cc (right):

https://codereview.chromium.org/21022018/diff/114001/chrome/browser/extension...
chrome/browser/extensions/api/sessions/sessions_api.cc:225: is_tab_entry(entry))
||
these should be able to fit on 1 line?

https://codereview.chromium.org/21022018/diff/114001/chrome/browser/extension...
chrome/browser/extensions/api/sessions/sessions_api.cc:266: profile())) {
looks like this will fit on 1 line

https://codereview.chromium.org/21022018/diff/114001/chrome/browser/extension...
chrome/browser/extensions/api/sessions/sessions_api.cc:274: SortTabsByRecency);
this too

https://codereview.chromium.org/21022018/diff/114001/chrome/browser/extension...
chrome/browser/extensions/api/sessions/sessions_api.cc:346: max_results =
*params->options->max_results.get();
no need for the .get() here. same for elsewhere with the *foo.get() calls (and
also the EXTENSION_FUNCTION_VALIDATE(foo)s).

https://codereview.chromium.org/21022018/diff/114001/chrome/common/extensions...
File chrome/common/extensions/api/sessions.json (right):

https://codereview.chromium.org/21022018/diff/114001/chrome/common/extensions...
chrome/common/extensions/api/sessions.json:25: "description": "The type of entry
to fetch. Omit this parameter to fetch all types of entries. Note: All
$ref:Device.sessions entries will be windows regardless of this value."
It's conceivable to me that one day we'll allow restoring individual tabs. In
that case, this comment would be out of date - but worse, we'll have documented
an incorrectness.

It's worth saying this, but on the getDevices method - for example, "the current
implementation of this method will only return [$ref:Window Windows], so
specifying <code>"tabs"</code> as an [$ref:Filter.entryType entryType] will
return an empty list of sessions". Or similar.

https://codereview.chromium.org/21022018/diff/114001/chrome/common/extensions...
chrome/common/extensions/api/sessions.json:55: "name": "options",
On 2013/08/19 17:53:12, dharcourt wrote:
> Nit: "options" -> "filter"?

I think that would result in slightly nicer generated code, too.

[Kristen Dwan, 2013-08-19 21:35:05]

Moved OWNERS file over. As for tests - the sessions_apitest.cc is what I
currently have. It does get a bit crazy with all of the mocks and having to add
sessions to profiles to test, etc. I don't know of any other cleaner way to do
it. If there is something else I should do to test, lmk. I was also going to
rewrite the sessions.js test (that you said is too slow which is why it doesn't
get run) if I still had time after all of the API gets submitted.

https://codereview.chromium.org/21022018/diff/87002/chrome/browser/extensions...
File chrome/browser/extensions/api/sessions/sessions_api.cc (right):

https://codereview.chromium.org/21022018/diff/87002/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.cc:288: break;
On 2013/08/19 19:34:40, kalman wrote:
> On 2013/08/16 22:03:06, Kristen Dwan wrote:
> > On 2013/08/15 20:16:24, kalman wrote:
> > >  but no need for the default: case.
> > 
> > there are 4 cases not dealt with here that we just consider state::none..
> > (default, inactive, detached, and end) that are causing compiler errors w/o
> > default.
> 
> That's ok, just be explicit about those 4 arguments. Fall-through cases etc.

Done.

https://codereview.chromium.org/21022018/diff/87002/chrome/browser/extensions...
File chrome/browser/extensions/api/sessions/sessions_api.h (right):

https://codereview.chromium.org/21022018/diff/87002/chrome/browser/extensions...
chrome/browser/extensions/api/sessions/sessions_api.h:26: struct SessionId;
On 2013/08/19 19:34:40, kalman wrote:
> forward declarations should still work for references?

Done.

https://codereview.chromium.org/21022018/diff/114001/chrome/browser/extension...
File chrome/browser/extensions/api/sessions/sessions_api.cc (right):

https://codereview.chromium.org/21022018/diff/114001/chrome/browser/extension...
chrome/browser/extensions/api/sessions/sessions_api.cc:225: is_tab_entry(entry))
||
On 2013/08/19 19:34:40, kalman wrote:
> these should be able to fit on 1 line?

.. the " {" hangs off the second if i limit them each to two lines.. updated to
look a tiny bit nicer... still ugly.

https://codereview.chromium.org/21022018/diff/114001/chrome/browser/extension...
chrome/browser/extensions/api/sessions/sessions_api.cc:266: profile())) {
On 2013/08/19 19:34:40, kalman wrote:
> looks like this will fit on 1 line

Done.

https://codereview.chromium.org/21022018/diff/114001/chrome/browser/extension...
chrome/browser/extensions/api/sessions/sessions_api.cc:274: SortTabsByRecency);
On 2013/08/19 19:34:40, kalman wrote:
> this too

wow don't know what was wrong with me .. sorry. must have had them tabbed in
somewhere and cut and moved and didn't notice..

https://codereview.chromium.org/21022018/diff/114001/chrome/browser/extension...
chrome/browser/extensions/api/sessions/sessions_api.cc:346: max_results =
*params->options->max_results.get();
On 2013/08/19 19:34:40, kalman wrote:
> no need for the .get() here. same for elsewhere with the *foo.get() calls (and
> also the EXTENSION_FUNCTION_VALIDATE(foo)s).

Done.

https://codereview.chromium.org/21022018/diff/114001/chrome/common/extensions...
File chrome/common/extensions/api/sessions.json (right):

https://codereview.chromium.org/21022018/diff/114001/chrome/common/extensions...
chrome/common/extensions/api/sessions.json:25: "description": "The type of entry
to fetch. Omit this parameter to fetch all types of entries. Note: All
$ref:Device.sessions entries will be windows regardless of this value."
On 2013/08/19 19:34:40, kalman wrote:
> It's conceivable to me that one day we'll allow restoring individual tabs. In
> that case, this comment would be out of date - but worse, we'll have
documented
> an incorrectness.
> 
> It's worth saying this, but on the getDevices method - for example, "the
current
> implementation of this method will only return [$ref:Window Windows], so
> specifying <code>"tabs"</code> as an [$ref:Filter.entryType entryType] will
> return an empty list of sessions". Or similar.

you already can restore an individual tab from a session. it's just the fact
that we always return the tabs in windows and you can choose any individual tab
from the window to restore it. the reason they're always encapsulated in a
window is because it would just be silly to have to return each individual tab
as a session *and* the same set of tabs encapsulated in their window in case you
wanted to restore the whole window (in the case that no filter was specified).

OR if you think it would be better, we can allow this filter and if tab is
specified just return every tab individually. but if nothing or window is
specified it will default to the usual return in windows.

(RESOLVED: removing filter by entryType. deemed unnecessary for either functions
that utilize filter.)

https://codereview.chromium.org/21022018/diff/114001/chrome/common/extensions...
chrome/common/extensions/api/sessions.json:28: },
On 2013/08/19 17:53:12, dharcourt wrote:
> FWIW, there are trailing spaces on this line (28) and this may (should?) cause
> trybots to fail.

Done.

https://codereview.chromium.org/21022018/diff/114001/chrome/common/extensions...
chrome/common/extensions/api/sessions.json:55: "name": "options",
On 2013/08/19 19:34:40, kalman wrote:
> On 2013/08/19 17:53:12, dharcourt wrote:
> > Nit: "options" -> "filter"?
> 
> I think that would result in slightly nicer generated code, too.

Done.

https://codereview.chromium.org/21022018/diff/114001/chrome/common/extensions...
chrome/common/extensions/api/sessions.json:76: "name": "options",
On 2013/08/19 17:53:12, dharcourt wrote:
> Nit: "options" -> "filter"?

Done.

[Nicolas Zea, 2013-08-19 23:54:40]

sync lgtm

https://codereview.chromium.org/21022018/diff/150001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service_mock.h (right):

https://codereview.chromium.org/21022018/diff/150001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service_mock.h:127:
MOCK_METHOD0(GetSessionModelAssociator,
nit: move this to be right above GetAllSignedInDeviceMock

[not at google - send to devlin, 2013-08-20 16:43:32]

https://codereview.chromium.org/21022018/diff/150001/chrome/common/extensions...
File chrome/common/extensions/api/sessions.json (right):

https://codereview.chromium.org/21022018/diff/150001/chrome/common/extensions...
chrome/common/extensions/api/sessions.json:7: "namespace": "sessions",
ok, looks like git has decided that this is a move from the permissions API.
ugh.

[commit-bot: I haz the power, 2013-08-21 06:48:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dwankri@chromium.org/21022018/43002

[commit-bot: I haz the power, 2013-08-21 08:34:43]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Kristen Dwan, 2013-08-21 14:48:51]

Rachel, could you check out chrome_browser_main_extra_parts_profile for owners?
thanks!

[rpetterson, 2013-08-21 14:52:48]

profiles/ LGTM
On Aug 21, 2013 7:48 AM, <dwankri@chromium.org> wrote:

> Rachel, could you check out chrome_browser_main_extra_**parts_profile for
> owners?
> thanks!
>
>
https://codereview.chromium.**org/21022018/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2013-08-21 15:00:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dwankri@chromium.org/21022018/43002

[commit-bot: I haz the power, 2013-08-21 15:00:44]

Failed to apply patch for
chrome/browser/extensions/extension_function_histogram_value.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/extensions/extension_function_histogram_value.h
  Hunk #2 FAILED at 610.
  1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/extensions/extension_function_histogram_value.h.rej

Patch:       chrome/browser/extensions/extension_function_histogram_value.h
Index: chrome/browser/extensions/extension_function_histogram_value.h
diff --git a/chrome/browser/extensions/extension_function_histogram_value.h
b/chrome/browser/extensions/extension_function_histogram_value.h
index
ad99eb8a58ea1870a57f1c0d40eab7cbfd1b5d43..737788ad0b847cefa02b0b1c48a1b239279d4284
100644
--- a/chrome/browser/extensions/extension_function_histogram_value.h
+++ b/chrome/browser/extensions/extension_function_histogram_value.h
@@ -459,8 +459,8 @@ enum HistogramValue {
   WEBVIEW_EXECUTESCRIPT,
   NOTIFICATIONS_UPDATE,
   NOTIFICATIONS_CLEAR,
-  SESSIONRESTORE_GETRECENTLYCLOSED,
-  SESSIONRESTORE_RESTORE,
+  DELETED_SESSIONRESTORE_GETRECENTLYCLOSED,
+  DELETED_SESSIONRESTORE_RESTORE,
   MANAGEMENT_UNINSTALLSELF,
   ECHOPRIVATE_GETOOBETIMESTAMP,
   FILEBROWSERPRIVATE_VALIDATEPATHNAMELENGTH,
@@ -610,6 +610,9 @@ enum HistogramValue {
   SOCKETS_UDP_GETJOINEDGROUPS,
   SIGNEDIN_DEVICES_GET,
   AUTOTESTPRIVATE_SIMULATEASANMEMORYBUG,
+  SESSIONS_GETRECENTLYCLOSED,
+  SESSIONS_GETDEVICES,
+  SESSIONS_RESTORE,
   ENUM_BOUNDARY // Last entry: Add new entries above.
 };

[commit-bot: I haz the power, 2013-08-21 16:34:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dwankri@chromium.org/21022018/216001

[commit-bot: I haz the power, 2013-08-21 16:59:38]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-08-22 16:51:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dwankri@chromium.org/21022018/75002

[commit-bot: I haz the power, 2013-08-22 19:21:57]

Change committed as 219066

[commit-bot: I haz the power, 2013-08-22 22:06:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dwankri@chromium.org/21022018/184017

[commit-bot: I haz the power, 2013-08-23 02:13:03]

Change committed as 219187