ios/chrome/browser/web/sad_tab_tab_helper.h - Issue 2807843002: Refactor creation of SadTabView into a tab helper object

Side by Side Diff: ios/chrome/browser/web/sad_tab_tab_helper.h

Issue 2807843002: Refactor creation of SadTabView into a tab helper object (Closed)

Patch Set: Created 3 years, 8 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/ui/sad_tab/sad_tab_view.mm ('K') | « ios/chrome/browser/web/BUILD.gn ('k') | ios/chrome/browser/web/sad_tab_tab_helper.mm » ('j') | ios/chrome/browser/web/sad_tab_tab_helper.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 #import "ios/chrome/browser/web/tab_helper_delegate.h"

	6 #import "ios/web/public/web_state/web_state_observer.h"

	7 #import "ios/web/public/web_state/web_state_user_data.h"

	8

	9 #if !defined(__has_feature) \|\| !__has_feature(objc_arc)

	10 #error "This file requires ARC support."

	11 #endif

	12

	13 class SadTabTabHelper : public web::WebStateUserData<SadTabTabHelper>,
	kkhorimoto 2017/04/10 22:31:09 Please add comments for this class Please add comments for this class PL 2017/04/11 01:35:21 Done, thanks! Show quoted text On 2017/04/10 22:31:09, kkhorimoto_ wrote: > Please add comments for this class Done, thanks!
	14 public web::WebStateObserver {

	15 public:

	16 explicit SadTabTabHelper(web::WebState* web_state);

	17 SadTabTabHelper(web::WebState* web_state, id<TabHelperDelegate> delegate);

	18 ~SadTabTabHelper() override;
	kkhorimoto 2017/04/10 22:31:10 If we want to enforce that this object is created If we want to enforce that this object is created via the static method below, let's move the constructors/destructor into private below. Also, pease add comments to these functions. PL 2017/04/11 01:35:21 Done: I don't need the first one, so I've taken th Show quoted text On 2017/04/10 22:31:10, kkhorimoto_ wrote: > If we want to enforce that this object is created via the static method below, > let's move the constructors/destructor into private below. Also, pease add > comments to these functions. Done: I don't need the first one, so I've taken that out and added a comment to the constructor and deconstructor.
	19

	20 static void CreateForWebState(web::WebState* web_state,
	kkhorimoto 2017/04/10 22:31:09 Needs a comment. Needs a comment. PL 2017/04/11 01:35:21 Done, thank you! Show quoted text On 2017/04/10 22:31:09, kkhorimoto_ wrote: > Needs a comment. Done, thank you!
	21 id<TabHelperDelegate> delegate);

	22

	23 // A TabHelperDelegate that can control aspects of this tab helper's behavior

	24 __weak id<TabHelperDelegate> delegate;
	kkhorimoto 2017/04/10 22:31:10 C++ ivars require a trailing underscore. Also, we C++ ivars require a trailing underscore. Also, we typically don't expose ivars publicly; can you move the ivar to the private portion of this class? Also, since we're setting the delegate in the constructor, do we even need to expose getters/setters? It seems like the delegate is only used internally within this helper. Eugene But (OOO till 7-30) 2017/04/10 22:56:28 More on Access Control here: https://google.github Show quoted text On 2017/04/10 22:31:10, kkhorimoto_ wrote: > C++ ivars require a trailing underscore. Also, we typically don't expose ivars > publicly; can you move the ivar to the private portion of this class? Also, > since we're setting the delegate in the constructor, do we even need to expose > getters/setters? It seems like the delegate is only used internally within this > helper. More on Access Control here: https://google.github.io/styleguide/cppguide.html#Access_Control PL 2017/04/11 01:35:21 Done! I've made the delegate variable private. I'm Show quoted text On 2017/04/10 22:31:10, kkhorimoto_ wrote: > C++ ivars require a trailing underscore. Also, we typically don't expose ivars > publicly; can you move the ivar to the private portion of this class? Also, > since we're setting the delegate in the constructor, do we even need to expose > getters/setters? It seems like the delegate is only used internally within this > helper. Done! I've made the delegate variable private. I'm not sure which getter/setter I might be able to move though, let me know if I've missed something here.
	25

	26 private:

	27 // Presents a new SadTabView via the web_state object

	28 void presentSadTab(const GURL& urlCausingFailure);
	kkhorimoto 2017/04/10 22:31:09 s/urlCausingFailure/url_causing_failure s/urlCausingFailure/url_causing_failure kkhorimoto 2017/04/10 22:31:10 s/presentSadTab/PresentSadTab s/presentSadTab/PresentSadTab PL 2017/04/11 01:35:21 Done. My Objective-C is showing, thanks! Show quoted text On 2017/04/10 22:31:09, kkhorimoto_ wrote: > s/urlCausingFailure/url_causing_failure Done. My Objective-C is showing, thanks!
	29

	30 // An overridden callback that notifies SadTabTabHelper of a renderer crash

	31 // via the web_state_observer superclass

	32 void RenderProcessGone() override;
	kkhorimoto 2017/04/10 22:31:09 Typically for overridden functions, we just commen Typically for overridden functions, we just comment with the superclass's name (see https://cs.chromium.org/chromium/src/ios/web/navigation/navigation_manager_im... for example) PL 2017/04/11 01:35:21 Done, thanks! Show quoted text On 2017/04/10 22:31:09, kkhorimoto_ wrote: > Typically for overridden functions, we just comment with the superclass's name > (see > https://cs.chromium.org/chromium/src/ios/web/navigation/navigation_manager_im... > for example) Done, thanks!
	33

	34 NSString* helperID();

	35 };

OLD	NEW