ios/shared/chrome/browser/tabs/web_state_list_serialisation.h - Issue 2771863002: [ios] Add free function to (de)serialise WebStateList.

Side by Side Diff: ios/shared/chrome/browser/tabs/web_state_list_serialisation.h

Issue 2771863002: [ios] Add free function to (de)serialise WebStateList. (Closed)

Patch Set: Created 3 years, 9 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

« ios/chrome/browser/tabs/tab_model.mm ('K') | « ios/shared/chrome/browser/tabs/BUILD.gn ('k') | ios/shared/chrome/browser/tabs/web_state_list_serialisation.mm » ('j') | ios/shared/chrome/browser/tabs/web_state_list_serialisation.mm » ('J')
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
(Empty)
	1 // Copyright 2017 The Chromium Authors. All rights reserved.

	2 // Use of this source code is governed by a BSD-style license that can be

	3 // found in the LICENSE file.

	4

	5 #ifndef IOS_SHARED_CHROME_BROWSER_TABS_WEB_STATE_LIST_SERIALISATION_H_

	6 #define IOS_SHARED_CHROME_BROWSER_TABS_WEB_STATE_LIST_SERIALISATION_H_

	7

	8 #import <Foundation/Foundation.h>

	9

	10 #import "ios/web/public/web_state/web_state.h"
	marq (ping after 24h) 2017/03/24 10:37:17 I'm sure you've explained this already, but assume I'm sure you've explained this already, but assume I have no useful attention span. Why can't these be WebStateList methods? I don't see any problematic dependencies. sdefresne 2017/03/27 16:14:44 Two reasons: 1. on desktop, those are not methods Show quoted text On 2017/03/24 10:37:17, marq wrote: > I'm sure you've explained this already, but assume I have no useful attention > span. Why can't these be WebStateList methods? I don't see any problematic > dependencies. Two reasons: 1. on desktop, those are not methods of TabStripModel, 2. they can be implemented as separate functions. Creating this as separate function also allow me to test them with test implementation of WebStateList. marq (ping after 24h) 2017/03/28 15:47:24 I'm not sure I find the first two reasons compelli Show quoted text On 2017/03/27 16:14:44, sdefresne wrote: > On 2017/03/24 10:37:17, marq wrote: > > I'm sure you've explained this already, but assume I have no useful attention > > span. Why can't these be WebStateList methods? I don't see any problematic > > dependencies. > > Two reasons: > 1. on desktop, those are not methods of TabStripModel, > 2. they can be implemented as separate functions. > > Creating this as separate function also allow me to test them with test > implementation of WebStateList. I'm not sure I find the first two reasons compelling, although the testing is nice. It's OK for now, but I'll probably want to talk about general patterns for free functions vs methods at some point.
	11

	12 @class CRWSessionStorage;

	13 class WebStateList;

	14

	15 // Returns an array of serialised sessions.

	16 // TODO(sdefresne): add unit tests.

	17 NSArray<CRWSessionStorage> SerializeWebStateList(

	18 WebStateList* web_state_list);

	19

	20 // Restores serialised sessions into the specified WebStateList. Returns the

	21 // number of WebStates restored.
	marq (ping after 24h) 2017/03/24 10:37:17 Prefer comments that explicitly name parameters: Prefer comments that explicitly name parameters: // Restores \|sessions\| into \|web_state_list\| ... Please add a comment explaining what \|param\| does. sdefresne 2017/03/27 16:14:44 Done. Show quoted text On 2017/03/24 10:37:17, marq wrote: > Prefer comments that explicitly name parameters: > > // Restores \|sessions\| into \|web_state_list\| ... > > Please add a comment explaining what \|param\| does. Done.
	22 // TODO(sdefresne): add unit tests.

	23 int DeserializeWebStateListFrom(WebStateList* web_state_list,
	marq (ping after 24h) 2017/03/24 10:37:17 Is the return value just a convenience, or is it a Is the return value just a convenience, or is it actually needed? That is, can a caller assume that the return value should always equal the different between \|web_state_list\|'s count() before and after calling this function? marq (ping after 24h) 2017/03/24 10:37:17 Naming nit: If you're going to use a preposition a Naming nit: If you're going to use a preposition at the end of the function name (such as "From"), then the referent of that preposition should be the first parameter. Right now this reads to me as "Deserialize a WebStateList from \|web_state_list_, also using \|param\| and \|sessions\|". So either drop the "From", or move \|sessions\| to be the first parameter. sdefresne 2017/03/27 16:14:44 Dropped "From". Show quoted text On 2017/03/24 10:37:17, marq wrote: > Naming nit: If you're going to use a preposition at the end of the function name > (such as "From"), then the referent of that preposition should be the first > parameter. Right now this reads to me as "Deserialize a WebStateList from > \|web_state_list_, also using \|param\| and \|sessions\|". > > So either drop the "From", or move \|sessions\| to be the first parameter. Dropped "From". sdefresne 2017/03/27 16:14:44 For convenience. Removed. Show quoted text On 2017/03/24 10:37:17, marq wrote: > Is the return value just a convenience, or is it actually needed? That is, can a > caller assume that the return value should always equal the different between > \|web_state_list\|'s count() before and after calling this function? For convenience. Removed.
	24 web::WebState::CreateParams params,

	25 NSArray<CRWSessionStorage> sessions);

	26

	27 #endif // IOS_SHARED_CHROME_BROWSER_TABS_WEB_STATE_LIST_SERIALISATION_H_

OLD	NEW