Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2807843002:
Refactor creation of SadTabView into a tab helper object (Closed)
|
Created:
3 years, 8 months ago by PL Modified:
3 years, 8 months ago Reviewers:
kkhorimoto, pkl (ping after 24h if needed), Eugene But (OOO till 7-30), sdefresne, peterlaurens CC:
chromium-reviews, ios-reviews+chrome_chromium.org, ios-reviews_chromium.org, Eugene But (OOO till 7-30), baxley+watch_chromium.org, pkl (ping after 24h if needed), ios-reviews+web_chromium.org, noyau+watch_chromium.org, marq+watch_chromium.org, huangml+watch_chromium.org, liaoyuke+watch_chromium.org, sdefresne+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor creation of SadTabView into a tab helper object and reduce
dependence on Tab.
BUG=Sad Tab works as expected
Review-Url: https://codereview.chromium.org/2807843002
Cr-Commit-Position: refs/heads/master@{#464269}
Committed: https://chromium.googlesource.com/chromium/src/+/ace0d4cd1d5616bd8ea03ce8ed314857c15d4b3a
Patch Set 1 #
Total comments: 51
Patch Set 2 : Change to delegate interface and other misc review changes. #
Total comments: 6
Patch Set 3 : Minor review changes #
Total comments: 50
Patch Set 4 : Further small review changes #
Total comments: 9
Patch Set 5 : Further review feedback #
Total comments: 2
Patch Set 6 : Adding ClearTransientContentView to TestWebState #Patch Set 7 : Attempt to fix build dependency #Messages
Total messages: 42 (21 generated)
The CQ bit was checked by peterlaurens@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Refactor creation of SadTabView into a tab helper object and reduce dependence on Tab. BUG=none ========== to ========== Refactor creation of SadTabView into a tab helper object and reduce dependence on Tab. BUG=none ==========
peterlaurens@chromium.org changed reviewers: + eugenebut@chromium.org, kkhorimoto@chromium.org, sdefresne@chromium.org
Hello, These changes refactor SadTabView creation into a helper object. There should be no user-visible impact from this change, but it should prepare us for any further SadTab changes. Thanks for your help! - Peter
Description was changed from ========== Refactor creation of SadTabView into a tab helper object and reduce dependence on Tab. BUG=none ========== to ========== Refactor creation of SadTabView into a tab helper object and reduce dependence on Tab. BUG=Sad Tab works as expected ==========
https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/ui/sad_t... File ios/chrome/browser/ui/sad_tab/sad_tab_view.mm (right): https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/ui/sad_t... ios/chrome/browser/ui/sad_tab/sad_tab_view.mm:394: if (self.reloadHandler) { We're already DCHECKing that the reload handler is non-nil in the initializer, so this extra check isn't needed. https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/sad_... File ios/chrome/browser/web/sad_tab_tab_helper.h (right): https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/sad_... ios/chrome/browser/web/sad_tab_tab_helper.h:13: class SadTabTabHelper : public web::WebStateUserData<SadTabTabHelper>, Please add comments for this class https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/sad_... ios/chrome/browser/web/sad_tab_tab_helper.h:18: ~SadTabTabHelper() override; 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. https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/sad_... ios/chrome/browser/web/sad_tab_tab_helper.h:20: static void CreateForWebState(web::WebState* web_state, Needs a comment. https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/sad_... ios/chrome/browser/web/sad_tab_tab_helper.h:24: __weak id<TabHelperDelegate> delegate; 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. https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/sad_... ios/chrome/browser/web/sad_tab_tab_helper.h:28: void presentSadTab(const GURL& urlCausingFailure); s/presentSadTab/PresentSadTab https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/sad_... ios/chrome/browser/web/sad_tab_tab_helper.h:28: void presentSadTab(const GURL& urlCausingFailure); s/urlCausingFailure/url_causing_failure https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/sad_... ios/chrome/browser/web/sad_tab_tab_helper.h:32: void RenderProcessGone() override; 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) https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/sad_... File ios/chrome/browser/web/sad_tab_tab_helper.mm (right): https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/sad_... ios/chrome/browser/web/sad_tab_tab_helper.mm:20: NSString* const SadTabTabHelperID = @"SadTabTabHelper"; If we continue with the delegate approach, please declare the extern'd const in sad_tab_helper.h. Also, s/SadTabHelperID/kSadTabHelperID https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/sad_... ios/chrome/browser/web/sad_tab_tab_helper.mm:23: : web::WebStateObserver(web_state) {} Since we only want these objects to be constructed via the static function below (which takes two parameters), we can just add a NOTREACHED() in this constructor. That way, users of this class who attempt to instantiate it via WebStateUserData::CreateForWebState() are forced to use SadTabHelper::CreateForWebState(). https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/sad_... ios/chrome/browser/web/sad_tab_tab_helper.mm:28: this->delegate = delegate; After you rename the |delegate| ivar to |delegate_|, this will be disambiguated and you can just assign directly in the initialization list. Also, we probably want to DCHECK that the delegate exists. https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/tab_... File ios/chrome/browser/web/tab_helper_delegate.h (right): https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/tab_... ios/chrome/browser/web/tab_helper_delegate.h:16: @protocol TabHelperDelegate<NSObject> Can you add comments elaborating the purpose of this delegate? I'm not sure we need a generalized delegate for all TabHelpers (like the naming here implies). From the implementation, it looks like this is specific to not creating SadTabViews for tabs that are not currently active; maybe we should create a delegate class specifically for SadTabHelper? "TabHelper" is a general pattern we use, but it isn't an actual class; I'm hesitant about creating a TabHelperDelegate since there isn't a specific class whose functionality we're delegating. https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/tab_... ios/chrome/browser/web/tab_helper_delegate.h:16: @protocol TabHelperDelegate<NSObject> Also, for C++ objects, we should use C++ delegates (see WebStateDelegate for an example).
https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/tab_... File ios/chrome/browser/web/tab_helper_delegate.h (right): https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/tab_... ios/chrome/browser/web/tab_helper_delegate.h:21: - (BOOL)tabHelperShouldBeActive:(NSString*)tabHelperID; Another reason I'm not a big fan of this is that "Active" doesn't really have a clear definition for TabHelpers in general. Even for SadTabHelper, it's a little ambiguous because the helper is still actively observing the WSO callbacks, but only updates the view hierarchy if this returns YES. I think a better solution would be a delegate specific to SadTabHelper with something like ShouldShowSadTabForWebState(). Another option (if we wanted to avoid adding C++ delegate classes, which are kinda clunky) would be to add additional functionality to SadTabHelper. You could probably implement the same behavior pretty easily by using WebStateListObserver to track whether the SadTabHelper's WebState is currently active, then decide whether to display the SadTabView using observed state.
https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/tabs/tab.mm File ios/chrome/browser/tabs/tab.mm (right): https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/tabs/tab... ios/chrome/browser/tabs/tab.mm:2077: - (BOOL)tabHelperShouldBeActive:(NSString*)tabHelperID { nit: Do you want to pragma mark this? https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/tabs/tab... ios/chrome/browser/tabs/tab.mm:2081: BOOL applicationIsActive = IsApplicationStateNotActive(state) == NO; Comparing BOOL variable to NO may not always work as expected on 32-bit platform: https://google.github.io/styleguide/objcguide.xml#BOOL_Pitfalls https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/sad_... File ios/chrome/browser/web/sad_tab_tab_helper.h (right): https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/sad_... ios/chrome/browser/web/sad_tab_tab_helper.h:24: __weak id<TabHelperDelegate> delegate; 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 https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/sad_... File ios/chrome/browser/web/sad_tab_tab_helper_unittest.mm (right): https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/sad_... ios/chrome/browser/web/sad_tab_tab_helper_unittest.mm:17: void ShowTransientContentView(CRWContentView* content_view) override { Do you want to update web::TestWebState instead of adding API here? https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/sad_... ios/chrome/browser/web/sad_tab_tab_helper_unittest.mm:41: // Test that the presentation-block can be triggered without a delegate nit: s/Test/Tests From Style Guide: "These comments should be descriptive ("Opens the file") rather than imperative ("Open the file");" https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/sad_... ios/chrome/browser/web/sad_tab_tab_helper_unittest.mm:44: new SadTabTabHelperTestWebState()); nit: How about this?: SadTabTabHelperTestWebState web_state; SadTabTabHelper::CreateForWebState(&web_state, nil); https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/sad_... ios/chrome/browser/web/sad_tab_tab_helper_unittest.mm:45: SadTabTabHelper::CreateForWebState(web_state.get(), nil); Do you want to document |nil| with comments?: SadTabTabHelper::CreateForWebState(web_state.get(), nil /*delegate*/); https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/tab_... File ios/chrome/browser/web/tab_helper_delegate.h (right): https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/tab_... ios/chrome/browser/web/tab_helper_delegate.h:21: - (BOOL)tabHelperShouldBeActive:(NSString*)tabHelperID; Do you want to rename this delegate to SadTabTabHelperDelegate and rename this method to?: - (BOOL)isTabCurrentlyDisplayedForTabHelper:(SadTabTabHelper*)tabHelper; The reasons are: 1.) delegate method should take caller as a first argument 2.) It is hard to understand how |tabHelperShouldBeActive:| should be implemented. 3.) You don't need TabHelperID with proposed approach
pkl@chromium.org changed reviewers: + pkl@chromium.org
drive-by https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/ui/sad_t... File ios/chrome/browser/ui/sad_tab/sad_tab_view.h (right): https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/ui/sad_t... ios/chrome/browser/ui/sad_tab/sad_tab_view.h:12: #if !defined(__has_feature) || !__has_feature(objc_arc) Should this ARC boilerplate be in the .h file?
Thanks for the guidance! I'll upload a new revision with these changes. https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/tabs/tab.mm File ios/chrome/browser/tabs/tab.mm (right): https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/tabs/tab... ios/chrome/browser/tabs/tab.mm:2077: - (BOOL)tabHelperShouldBeActive:(NSString*)tabHelperID { On 2017/04/10 22:56:28, Eugene But wrote: > nit: Do you want to pragma mark this? Done, thanks! https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/tabs/tab... ios/chrome/browser/tabs/tab.mm:2081: BOOL applicationIsActive = IsApplicationStateNotActive(state) == NO; On 2017/04/10 22:56:28, Eugene But wrote: > Comparing BOOL variable to NO may not always work as expected on 32-bit > platform: > https://google.github.io/styleguide/objcguide.xml#BOOL_Pitfalls Done: Tweaked as discussed, thanks! https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/ui/sad_t... File ios/chrome/browser/ui/sad_tab/sad_tab_view.h (right): https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/ui/sad_t... ios/chrome/browser/ui/sad_tab/sad_tab_view.h:12: #if !defined(__has_feature) || !__has_feature(objc_arc) On 2017/04/11 00:09:21, pklpkl wrote: > Should this ARC boilerplate be in the .h file? Done: Think I was just overzealous here, thanks! https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/ui/sad_t... File ios/chrome/browser/ui/sad_tab/sad_tab_view.mm (right): https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/ui/sad_t... ios/chrome/browser/ui/sad_tab/sad_tab_view.mm:394: if (self.reloadHandler) { On 2017/04/10 22:31:09, kkhorimoto_ wrote: > We're already DCHECKing that the reload handler is non-nil in the initializer, > so this extra check isn't needed. Happy to take this check out if you feel strongly here, but here's my thinking: • DCHECK (I think) only blows up if we're in Debug, but we'll always blow up with a nil handler in shipping software so I think we should be wary of considering that DCHECK will catch everything here? • It's not possible to guarantee that this class won't change subtly in the future where the handler will be nullable etc. so I'd rather take the precaution now. What do you think? https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/sad_... File ios/chrome/browser/web/sad_tab_tab_helper.h (right): https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/sad_... ios/chrome/browser/web/sad_tab_tab_helper.h:13: class SadTabTabHelper : public web::WebStateUserData<SadTabTabHelper>, On 2017/04/10 22:31:09, kkhorimoto_ wrote: > Please add comments for this class Done, thanks! https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/sad_... ios/chrome/browser/web/sad_tab_tab_helper.h:18: ~SadTabTabHelper() override; 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. https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/sad_... ios/chrome/browser/web/sad_tab_tab_helper.h:20: static void CreateForWebState(web::WebState* web_state, On 2017/04/10 22:31:09, kkhorimoto_ wrote: > Needs a comment. Done, thank you! https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/sad_... ios/chrome/browser/web/sad_tab_tab_helper.h:24: __weak id<TabHelperDelegate> delegate; 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. https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/sad_... ios/chrome/browser/web/sad_tab_tab_helper.h:28: void presentSadTab(const GURL& urlCausingFailure); On 2017/04/10 22:31:09, kkhorimoto_ wrote: > s/urlCausingFailure/url_causing_failure Done. My Objective-C is showing, thanks! https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/sad_... ios/chrome/browser/web/sad_tab_tab_helper.h:32: void RenderProcessGone() override; 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! https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/sad_... File ios/chrome/browser/web/sad_tab_tab_helper.mm (right): https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/sad_... ios/chrome/browser/web/sad_tab_tab_helper.mm:20: NSString* const SadTabTabHelperID = @"SadTabTabHelper"; On 2017/04/10 22:31:10, kkhorimoto_ wrote: > If we continue with the delegate approach, please declare the extern'd const in > sad_tab_helper.h. Also, s/SadTabHelperID/kSadTabHelperID Done: I removed the notion of a tabID altogether. https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/sad_... ios/chrome/browser/web/sad_tab_tab_helper.mm:23: : web::WebStateObserver(web_state) {} On 2017/04/10 22:31:10, kkhorimoto_ wrote: > Since we only want these objects to be constructed via the static function below > (which takes two parameters), we can just add a NOTREACHED() in this > constructor. That way, users of this class who attempt to instantiate it via > WebStateUserData::CreateForWebState() are forced to use > SadTabHelper::CreateForWebState(). Done: I've removed these entirely (is this satisfactory?). https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/sad_... ios/chrome/browser/web/sad_tab_tab_helper.mm:28: this->delegate = delegate; On 2017/04/10 22:31:10, kkhorimoto_ wrote: > After you rename the |delegate| ivar to |delegate_|, this will be disambiguated > and you can just assign directly in the initialization list. Also, we probably > want to DCHECK that the delegate exists. Thanks! I wanted to provide for some flexibility and not force a delegate to be provided. How do you feel about leaving that option open? https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/sad_... File ios/chrome/browser/web/sad_tab_tab_helper_unittest.mm (right): https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/sad_... ios/chrome/browser/web/sad_tab_tab_helper_unittest.mm:17: void ShowTransientContentView(CRWContentView* content_view) override { On 2017/04/10 22:56:28, Eugene But wrote: > Do you want to update web::TestWebState instead of adding API here? I could do that no problem, but here's why I did this: it's not possible to update TestWebState to do the right thing here without adding some flag (as it just calls through to a CRWebController so no state change in the WebState is currently triggered). I didn't see other cases of flags being added like this to the main TestWebState so I created this subclass just for the purpose of this test rather than complicate the superclass for everyone. I saw this pattern used in other places such as reading_list_web_state_observer_unittest.mm. What do you think about keeping this here? https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/sad_... ios/chrome/browser/web/sad_tab_tab_helper_unittest.mm:41: // Test that the presentation-block can be triggered without a delegate On 2017/04/10 22:56:28, Eugene But wrote: > nit: s/Test/Tests > > From Style Guide: "These comments should be descriptive ("Opens the file") > rather than imperative ("Open the file");" > https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... Done, thanks! https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/sad_... ios/chrome/browser/web/sad_tab_tab_helper_unittest.mm:44: new SadTabTabHelperTestWebState()); On 2017/04/10 22:56:28, Eugene But wrote: > nit: How about this?: > SadTabTabHelperTestWebState web_state; > SadTabTabHelper::CreateForWebState(&web_state, nil); Done; this is great thanks! https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/sad_... ios/chrome/browser/web/sad_tab_tab_helper_unittest.mm:45: SadTabTabHelper::CreateForWebState(web_state.get(), nil); On 2017/04/10 22:56:28, Eugene But wrote: > Do you want to document |nil| with comments?: > > SadTabTabHelper::CreateForWebState(web_state.get(), nil /*delegate*/); Done! https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/tab_... File ios/chrome/browser/web/tab_helper_delegate.h (right): https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/tab_... ios/chrome/browser/web/tab_helper_delegate.h:16: @protocol TabHelperDelegate<NSObject> On 2017/04/10 22:31:10, kkhorimoto_ wrote: > Also, for C++ objects, we should use C++ delegates (see WebStateDelegate for an > example). Done, thanks! Changing this as discussed offline: Continue using a delegate but abandon an attempt to make a generic tab helper delegate pattern here. https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/tab_... ios/chrome/browser/web/tab_helper_delegate.h:21: - (BOOL)tabHelperShouldBeActive:(NSString*)tabHelperID; On 2017/04/10 22:44:14, kkhorimoto_ wrote: > Another reason I'm not a big fan of this is that "Active" doesn't really have a > clear definition for TabHelpers in general. Even for SadTabHelper, it's a > little ambiguous because the helper is still actively observing the WSO > callbacks, but only updates the view hierarchy if this returns YES. I think a > better solution would be a delegate specific to SadTabHelper with something like > ShouldShowSadTabForWebState(). Another option (if we wanted to avoid adding C++ > delegate classes, which are kinda clunky) would be to add additional > functionality to SadTabHelper. You could probably implement the same behavior > pretty easily by using WebStateListObserver to track whether the SadTabHelper's > WebState is currently active, then decide whether to display the SadTabView > using observed state. Done, thanks! Changing this as discussed offline: Continue using a delegate but abandon an attempt to make a generic tab helper delegate pattern here.
https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/ui/sad_t... File ios/chrome/browser/ui/sad_tab/sad_tab_view.mm (right): https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/ui/sad_t... ios/chrome/browser/ui/sad_tab/sad_tab_view.mm:394: if (self.reloadHandler) { On 2017/04/11 01:35:21, PL wrote: > On 2017/04/10 22:31:09, kkhorimoto_ wrote: > > We're already DCHECKing that the reload handler is non-nil in the initializer, > > so this extra check isn't needed. > > Happy to take this check out if you feel strongly here, but here's my thinking: > > • DCHECK (I think) only blows up if we're in Debug, but we'll always blow up > with a nil handler in shipping software so I think we should be wary of > considering that DCHECK will catch everything here? > > • It's not possible to guarantee that this class won't change subtly in the > future where the handler will be nullable etc. so I'd rather take the precaution > now. > > What do you think? The Chromium style guide says that if we're DCHECKing something, we should not add code that handles when the DCHECK fails: https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... That being said, you're right that DCHECKing in the initializer might not be sufficient if the implementation of this class changes and the reload handler is nil'd out somehow. However, that is not a state we ever want to get this class into (i.e. a reload button that doesn't do anything), so how about adding an additional DCHECK here in this function, then calling |reloadHandler| immediately afterward. https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/sad_... File ios/chrome/browser/web/sad_tab_tab_helper.mm (right): https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/sad_... ios/chrome/browser/web/sad_tab_tab_helper.mm:23: : web::WebStateObserver(web_state) {} On 2017/04/11 01:35:21, PL wrote: > On 2017/04/10 22:31:10, kkhorimoto_ wrote: > > Since we only want these objects to be constructed via the static function > below > > (which takes two parameters), we can just add a NOTREACHED() in this > > constructor. That way, users of this class who attempt to instantiate it via > > WebStateUserData::CreateForWebState() are forced to use > > SadTabHelper::CreateForWebState(). > > Done: I've removed these entirely (is this satisfactory?). A SadTabTabHelper could still be created via WebStatUserData::CreateForWebState(), which will call the superclass's implementation of this constructor. If that were to occur, then |delegate_| would be a garbage value since C++ objects don't initialize ivars to nullptrs. This would cause a crash when if the WSO callback was ever received. It's easier to debug calling a constructor you aren't supposed to use that has NOTREACHED() than to figure out why |delegate_| would be a garbage value if this were to happen, so I think there's value in keeping this constructor around and just adding a NOTREACHED(). https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/sad_... ios/chrome/browser/web/sad_tab_tab_helper.mm:28: this->delegate = delegate; On 2017/04/11 01:35:21, PL wrote: > On 2017/04/10 22:31:10, kkhorimoto_ wrote: > > After you rename the |delegate| ivar to |delegate_|, this will be > disambiguated > > and you can just assign directly in the initialization list. Also, we > probably > > want to DCHECK that the delegate exists. > > Thanks! I wanted to provide for some flexibility and not force a delegate to > be provided. How do you feel about leaving that option open? There's definitely some benefit to using defensive programming (i.e. checking that delegate is non-nil before calling) vs using assertions/DCHECKs if this class were to be used outside of the Chrome app. However, the Chromium culture tends to lean more in the direction of using DCHECKs to force the behavior we want, as it makes the intention of the class's behavior more apparent. Using defensive programming will sometimes introduce subtle bugs when an object isn't set the way we expect, and it's a lot harder to hunt those down rather than just crashing on a DCHECK, which clearly denotes the author's intention. https://codereview.chromium.org/2807843002/diff/20001/ios/chrome/browser/web/... File ios/chrome/browser/web/sad_tab_tab_helper.h (right): https://codereview.chromium.org/2807843002/diff/20001/ios/chrome/browser/web/... ios/chrome/browser/web/sad_tab_tab_helper.h:14: // SadTabView view appropriately Please finish comments with a period here and elsewhere in this CL. https://codereview.chromium.org/2807843002/diff/20001/ios/chrome/browser/web/... ios/chrome/browser/web/sad_tab_tab_helper.h:25: // providing an optional delegate Do we want the delegate to be optional? I can't think of a reason why we would want to show a SadTabView for a Tab that isn't currently visible.... https://codereview.chromium.org/2807843002/diff/20001/ios/chrome/browser/web/... File ios/chrome/browser/web/sad_tab_tab_helper.mm (right): https://codereview.chromium.org/2807843002/diff/20001/ios/chrome/browser/web/... ios/chrome/browser/web/sad_tab_tab_helper.mm:24: : web::WebStateObserver(web_state) { You can use initialization lists to set |delegate_| as well: SadTabTabHelper::SadTabTabHelper(web::WebState* web_state, id<SadTabTabHelperDelegate> delegate) : web::WebStateObserver(web_state) , delegate_(delegate) {}
Much appreciated, I'll make these changes and upload a new version shortly. https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/ui/sad_t... File ios/chrome/browser/ui/sad_tab/sad_tab_view.mm (right): https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/ui/sad_t... ios/chrome/browser/ui/sad_tab/sad_tab_view.mm:394: if (self.reloadHandler) { On 2017/04/11 02:13:15, kkhorimoto_ wrote: > On 2017/04/11 01:35:21, PL wrote: > > On 2017/04/10 22:31:09, kkhorimoto_ wrote: > > > We're already DCHECKing that the reload handler is non-nil in the > initializer, > > > so this extra check isn't needed. > > > > Happy to take this check out if you feel strongly here, but here's my > thinking: > > > > • DCHECK (I think) only blows up if we're in Debug, but we'll always blow up > > with a nil handler in shipping software so I think we should be wary of > > considering that DCHECK will catch everything here? > > > > • It's not possible to guarantee that this class won't change subtly in the > > future where the handler will be nullable etc. so I'd rather take the > precaution > > now. > > > > What do you think? > > The Chromium style guide says that if we're DCHECKing something, we should not > add code that handles when the DCHECK fails: > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > That being said, you're right that DCHECKing in the initializer might not be > sufficient if the implementation of this class changes and the reload handler is > nil'd out somehow. However, that is not a state we ever want to get this class > into (i.e. a reload button that doesn't do anything), so how about adding an > additional DCHECK here in this function, then calling |reloadHandler| > immediately afterward. Done! I'm a little nervous but I trust your guidance! https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/sad_... File ios/chrome/browser/web/sad_tab_tab_helper.mm (right): https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/sad_... ios/chrome/browser/web/sad_tab_tab_helper.mm:23: : web::WebStateObserver(web_state) {} On 2017/04/11 02:13:15, kkhorimoto_ wrote: > On 2017/04/11 01:35:21, PL wrote: > > On 2017/04/10 22:31:10, kkhorimoto_ wrote: > > > Since we only want these objects to be constructed via the static function > > below > > > (which takes two parameters), we can just add a NOTREACHED() in this > > > constructor. That way, users of this class who attempt to instantiate it > via > > > WebStateUserData::CreateForWebState() are forced to use > > > SadTabHelper::CreateForWebState(). > > > > Done: I've removed these entirely (is this satisfactory?). > > A SadTabTabHelper could still be created via > WebStatUserData::CreateForWebState(), which will call the superclass's > implementation of this constructor. If that were to occur, then |delegate_| > would be a garbage value since C++ objects don't initialize ivars to nullptrs. > This would cause a crash when if the WSO callback was ever received. It's > easier to debug calling a constructor you aren't supposed to use that has > NOTREACHED() than to figure out why |delegate_| would be a garbage value if this > were to happen, so I think there's value in keeping this constructor around and > just adding a NOTREACHED(). Done. Thanks! https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/sad_... ios/chrome/browser/web/sad_tab_tab_helper.mm:28: this->delegate = delegate; On 2017/04/11 02:13:15, kkhorimoto_ wrote: > On 2017/04/11 01:35:21, PL wrote: > > On 2017/04/10 22:31:10, kkhorimoto_ wrote: > > > After you rename the |delegate| ivar to |delegate_|, this will be > > disambiguated > > > and you can just assign directly in the initialization list. Also, we > > probably > > > want to DCHECK that the delegate exists. > > > > Thanks! I wanted to provide for some flexibility and not force a delegate to > > be provided. How do you feel about leaving that option open? > > There's definitely some benefit to using defensive programming (i.e. checking > that delegate is non-nil before calling) vs using assertions/DCHECKs if this > class were to be used outside of the Chrome app. However, the Chromium culture > tends to lean more in the direction of using DCHECKs to force the behavior we > want, as it makes the intention of the class's behavior more apparent. Using > defensive programming will sometimes introduce subtle bugs when an object isn't > set the way we expect, and it's a lot harder to hunt those down rather than just > crashing on a DCHECK, which clearly denotes the author's intention. Done; makes sense, thanks. https://codereview.chromium.org/2807843002/diff/20001/ios/chrome/browser/web/... File ios/chrome/browser/web/sad_tab_tab_helper.h (right): https://codereview.chromium.org/2807843002/diff/20001/ios/chrome/browser/web/... ios/chrome/browser/web/sad_tab_tab_helper.h:14: // SadTabView view appropriately On 2017/04/11 02:13:15, kkhorimoto_ wrote: > Please finish comments with a period here and elsewhere in this CL. Done! (Maybe this is something we can have git cl format do in the future). https://codereview.chromium.org/2807843002/diff/20001/ios/chrome/browser/web/... ios/chrome/browser/web/sad_tab_tab_helper.h:25: // providing an optional delegate On 2017/04/11 02:13:15, kkhorimoto_ wrote: > Do we want the delegate to be optional? I can't think of a reason why we would > want to show a SadTabView for a Tab that isn't currently visible.... Done. This made more sense when I had the notion of a general delegate interface. This doesn't really have a reason now, so I've just taken it out. https://codereview.chromium.org/2807843002/diff/20001/ios/chrome/browser/web/... File ios/chrome/browser/web/sad_tab_tab_helper.mm (right): https://codereview.chromium.org/2807843002/diff/20001/ios/chrome/browser/web/... ios/chrome/browser/web/sad_tab_tab_helper.mm:24: : web::WebStateObserver(web_state) { On 2017/04/11 02:13:15, kkhorimoto_ wrote: > You can use initialization lists to set |delegate_| as well: > > SadTabTabHelper::SadTabTabHelper(web::WebState* web_state, > id<SadTabTabHelperDelegate> delegate) > : web::WebStateObserver(web_state) > , delegate_(delegate) {} Done! (That's cool).
https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/sad_tab/sad_tab_view.mm (right): https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/sad_tab/sad_tab_view.mm:57: @property(nonatomic, copy) ProceduralBlock reloadHandler; You should leave this property as readonly to avoid the need for DCHECK when the value is accessed (since there is a DCHECK in the initializer, and if the property is readonly, we know that it cannot be changed after that via the property). https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/sad_tab/sad_tab_view.mm:59: @property(nonatomic, strong) UIView* containerView; Please leave as readonly. I think "strong" is optional here (as it is the default), so either @property(nonatomic, readonly) UIView* containerView; or @property(nonatomic, readonly, strong) UIView* containerView; https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/sad_tab/sad_tab_view.mm:61: @property(nonatomic, strong) UIImageView* imageView; ditto https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/sad_tab/sad_tab_view.mm:63: @property(nonatomic, strong) UILabel* titleLabel; ditto https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/sad_tab/sad_tab_view.mm:65: @property(nonatomic, strong) UILabel* messageLabel; ditto https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/sad_tab/sad_tab_view.mm:67: @property(nonatomic, strong) UILabel* helpLabel; ditto https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/sad_tab/sad_tab_view.mm:68: @property(nonatomic, strong) LabelLinkController* helpLabelLinkController; ditto https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/sad_tab/sad_tab_view.mm:70: @property(nonatomic, strong) MDCFlatButton* reloadButton; ditto https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/sad_tab/sad_tab_view.mm:394: DCHECK(self.reloadHandler); Prefer to put DCHECK in setters. If this DCHECK fire, then it does not add any additional information (as calling the nil block will also fail and cause a callstack to be collected). The important callstack would be the one when this property is set to nil. In addition, if the property is left readonly, then it is known that it can only be changed by the backing ivar, which is only assigned in the initializer (where it is DCHECK-ed), so there is no need for an additional DCHECK here. https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/web/... File ios/chrome/browser/web/sad_tab_tab_helper.h (right): https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/web/... ios/chrome/browser/web/sad_tab_tab_helper.h:30: explicit SadTabTabHelper(web::WebState* web_state); You should be able to remove this constructor. Note that if you remove the constructor, calling "SadTabTabHelper::CreateForWebState(web_state)" will cause a compilation error (constructor does not exists) which is better than a runtime error (NOTREACHED). https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/web/... File ios/chrome/browser/web/sad_tab_tab_helper_delegate.h (right): https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/web/... ios/chrome/browser/web/sad_tab_tab_helper_delegate.h:5: #ifndef tab_helper_delegate_h This is not using the correct header guards. Please use src/tools/boilerplate.py script to create file with the correct header guards.
https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/sad_... File ios/chrome/browser/web/sad_tab_tab_helper_unittest.mm (right): https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/sad_... ios/chrome/browser/web/sad_tab_tab_helper_unittest.mm:17: void ShowTransientContentView(CRWContentView* content_view) override { On 2017/04/11 01:35:21, PL wrote: > On 2017/04/10 22:56:28, Eugene But wrote: > > Do you want to update web::TestWebState instead of adding API here? > > I could do that no problem, but here's why I did this: it's not possible to > update TestWebState to do the right thing here without adding some flag (as it > just calls through to a CRWebController so no state change in the WebState is > currently triggered). Are you sure that web::TestWebState uses CRWWebController? > I didn't see other cases of flags being added like this to the main TestWebState > so I created this subclass just for the purpose of this test rather than > complicate the superclass for everyone. I saw this pattern used in other > places such as reading_list_web_state_observer_unittest.mm. > > What do you think about keeping this here? This code could be useful in other places. https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/tabs... File ios/chrome/browser/tabs/tab.mm (right): https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/tabs... ios/chrome/browser/tabs/tab.mm:2080: BOOL shouldBeActive = YES; How about this?: UIApplicationState state = UIApplication.sharedApplication.applicationState; return visible_ && !IsApplicationStateNotActive(state); https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/web/... File ios/chrome/browser/web/sad_tab_tab_helper.h (right): https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/web/... ios/chrome/browser/web/sad_tab_tab_helper.h:5: #import "ios/chrome/browser/web/sad_tab_tab_helper_delegate.h" nit: Would predeclaration be sufficient here? C++ Guides advocate for includes and Chromium Style Guide for predeclarations. Chromium Style Guide overrides C++ Style Guide. https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/web/... ios/chrome/browser/web/sad_tab_tab_helper.h:18: // Create a SadTabTabHelper and attach it to a specific web_state object, s/Create/Creates s/attach/attaches https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/web/... ios/chrome/browser/web/sad_tab_tab_helper.h:32: // Deconstructor for SadTabTabHelper. nit: do we need this comment? Technically it is "Destructor", but that's quite obvious from the code. https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/web/... ios/chrome/browser/web/sad_tab_tab_helper.h:36: __weak id<SadTabTabHelperDelegate> delegate_; Could you please move data member to the bottom (after methods). https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/web/... File ios/chrome/browser/web/sad_tab_tab_helper.mm (right): https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/web/... ios/chrome/browser/web/sad_tab_tab_helper.mm:9: #import "base/strings/sys_string_conversions.h" s/import/include (because Objective-C code is guarded by ifdefs) https://google.github.io/styleguide/objcguide.xml#_import_and__include https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/web/... ios/chrome/browser/web/sad_tab_tab_helper.mm:20: NSString* const SadTabTabHelperID = @"SadTabTabHelper"; Do we still need this? https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/web/... ios/chrome/browser/web/sad_tab_tab_helper.mm:46: GURL lastCommittedURL = web_state()->GetLastCommittedURL(); s/lastCommittedURL/last_committed_url (because this is C++ method) Same comment for sadTabView and contentView https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/web/... ios/chrome/browser/web/sad_tab_tab_helper.mm:52: SadTabView* sadTabView = [[SadTabView alloc] initWithReloadHandler:^{ s/sadTabView/sad_tab_view https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/web/... File ios/chrome/browser/web/sad_tab_tab_helper_delegate.h (right): https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/web/... ios/chrome/browser/web/sad_tab_tab_helper_delegate.h:18: // Returns whether the tab for the tab helper is visible, allowing differences Do you want to elaborate what |visible| means in this context (app is in active state and tab is the frontmost)? https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/web/... File ios/chrome/browser/web/sad_tab_tab_helper_unittest.mm (right): https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/web/... ios/chrome/browser/web/sad_tab_tab_helper_unittest.mm:22: bool DidShowTransientContentView() { return didShowTransientContentView; } How about |bool is_showing_transient_content_view() const { return is_showing_transient_content_view_; }|? https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/web/... ios/chrome/browser/web/sad_tab_tab_helper_unittest.mm:25: bool didShowTransientContentView; How about: bool is_showing_transient_content_view_ = true; ? https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/web/... ios/chrome/browser/web/sad_tab_tab_helper_unittest.mm:44: SadTabTabHelper::CreateForWebState(&web_state, delegate); Optional nit: Do you want to create SadTabTabHelperTest test fixture? Like this: class SadTabTabHelperTest : public PlatformTest { protected: SadTabTabHelperTest() : delegate_([[TabHelperTestDelegate alloc] init]) { SadTabTabHelper::CreateForWebState(&web_state, delegate); } TabHelperTestDelegate* delegate_; SadTabTabHelperTestWebState web_state_; };
https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/tabs... File ios/chrome/browser/tabs/tab.mm (right): https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/tabs... ios/chrome/browser/tabs/tab.mm:2080: BOOL shouldBeActive = YES; On 2017/04/11 15:17:00, Eugene But wrote: > How about this?: > > UIApplicationState state = UIApplication.sharedApplication.applicationState; > return visible_ && !IsApplicationStateNotActive(state); > +1, or at least rename |shouldBeActive| to |isTabVisible| https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/web/... File ios/chrome/browser/web/sad_tab_tab_helper.h (right): https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/web/... ios/chrome/browser/web/sad_tab_tab_helper.h:30: explicit SadTabTabHelper(web::WebState* web_state); On 2017/04/11 08:17:39, sdefresne wrote: > You should be able to remove this constructor. > > Note that if you remove the constructor, calling > "SadTabTabHelper::CreateForWebState(web_state)" will cause a compilation error > (constructor does not exists) which is better than a runtime error (NOTREACHED). Are you sure it would cause a compilation error? I would assume that it would just call the superclass's constructor and leave |delegate_| uninitialized in this subclass, which is why I suggested keeping this constructor and adding a NOTREACHED. Peter, can you verify whether calling SadTabTabHelper::CreateForWebState(WebState*) causes a compilation error and remove if it does? Sorry for the back and forth on this!
peterlaurens@google.com changed reviewers: + peterlaurens@google.com
Thanks for the guidance! New revision on it's way. https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/tabs... File ios/chrome/browser/tabs/tab.mm (right): https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/tabs... ios/chrome/browser/tabs/tab.mm:2080: BOOL shouldBeActive = YES; On 2017/04/11 15:17:00, Eugene But wrote: > How about this?: > > UIApplicationState state = UIApplication.sharedApplication.applicationState; > return visible_ && !IsApplicationStateNotActive(state); > Done, thanks! To explain why I wrote this like this: • I wanted to make this as readable as possible and avoid a double-negative • I have preferred to use local variables that are set and then returned, as I've found that can help logging or making quick changes to force a state during debugging. I understand that this is less applicable here though, so happy to make this change! https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/sad_tab/sad_tab_view.mm (right): https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/sad_tab/sad_tab_view.mm:57: @property(nonatomic, copy) ProceduralBlock reloadHandler; On 2017/04/11 08:17:39, sdefresne wrote: > You should leave this property as readonly to avoid the need for DCHECK when the > value is accessed (since there is a DCHECK in the initializer, and if the > property is readonly, we know that it cannot be changed after that via the > property). Done. https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/sad_tab/sad_tab_view.mm:59: @property(nonatomic, strong) UIView* containerView; On 2017/04/11 08:17:39, sdefresne wrote: > Please leave as readonly. I think "strong" is optional here (as it is the > default), so either > > @property(nonatomic, readonly) UIView* containerView; > > or > > @property(nonatomic, readonly, strong) UIView* containerView; Done. https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/sad_tab/sad_tab_view.mm:61: @property(nonatomic, strong) UIImageView* imageView; On 2017/04/11 08:17:39, sdefresne wrote: > ditto Done. https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/sad_tab/sad_tab_view.mm:63: @property(nonatomic, strong) UILabel* titleLabel; On 2017/04/11 08:17:39, sdefresne wrote: > ditto Done. https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/sad_tab/sad_tab_view.mm:65: @property(nonatomic, strong) UILabel* messageLabel; On 2017/04/11 08:17:39, sdefresne wrote: > ditto Done. https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/sad_tab/sad_tab_view.mm:67: @property(nonatomic, strong) UILabel* helpLabel; On 2017/04/11 08:17:39, sdefresne wrote: > ditto Done. https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/sad_tab/sad_tab_view.mm:68: @property(nonatomic, strong) LabelLinkController* helpLabelLinkController; On 2017/04/11 08:17:39, sdefresne wrote: > ditto Done. https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/sad_tab/sad_tab_view.mm:70: @property(nonatomic, strong) MDCFlatButton* reloadButton; On 2017/04/11 08:17:39, sdefresne wrote: > ditto Done. https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/sad_tab/sad_tab_view.mm:394: DCHECK(self.reloadHandler); On 2017/04/11 08:17:39, sdefresne wrote: > Prefer to put DCHECK in setters. If this DCHECK fire, then it does not add any > additional information (as calling the nil block will also fail and cause a > callstack to be collected). The important callstack would be the one when this > property is set to nil. > > In addition, if the property is left readonly, then it is known that it can only > be changed by the backing ivar, which is only assigned in the initializer (where > it is DCHECK-ed), so there is no need for an additional DCHECK here. Done. Thanks! https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/web/... File ios/chrome/browser/web/sad_tab_tab_helper.h (right): https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/web/... ios/chrome/browser/web/sad_tab_tab_helper.h:5: #import "ios/chrome/browser/web/sad_tab_tab_helper_delegate.h" On 2017/04/11 15:17:00, Eugene But wrote: > nit: Would predeclaration be sufficient here? > > C++ Guides advocate for includes and Chromium Style Guide for predeclarations. > Chromium Style Guide overrides C++ Style Guide. Yes! Done, thanks! https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/web/... ios/chrome/browser/web/sad_tab_tab_helper.h:18: // Create a SadTabTabHelper and attach it to a specific web_state object, On 2017/04/11 15:17:00, Eugene But wrote: > s/Create/Creates s/attach/attaches Done, thanks. https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/web/... ios/chrome/browser/web/sad_tab_tab_helper.h:30: explicit SadTabTabHelper(web::WebState* web_state); On 2017/04/11 17:38:02, kkhorimoto_ wrote: > On 2017/04/11 08:17:39, sdefresne wrote: > > You should be able to remove this constructor. > > > > Note that if you remove the constructor, calling > > "SadTabTabHelper::CreateForWebState(web_state)" will cause a compilation error > > (constructor does not exists) which is better than a runtime error > (NOTREACHED). > > Are you sure it would cause a compilation error? I would assume that it would > just call the superclass's constructor and leave |delegate_| uninitialized in > this subclass, which is why I suggested keeping this constructor and adding a > NOTREACHED. Peter, can you verify whether calling > SadTabTabHelper::CreateForWebState(WebState*) causes a compilation error and > remove if it does? Sorry for the back and forth on this! Done. Can confirm that a compilation error occurs (even if including headers from the superclasses). I'll take this out, thanks! https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/web/... ios/chrome/browser/web/sad_tab_tab_helper.h:32: // Deconstructor for SadTabTabHelper. On 2017/04/11 15:17:00, Eugene But wrote: > nit: do we need this comment? Technically it is "Destructor", but that's quite > obvious from the code. Done. https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/web/... ios/chrome/browser/web/sad_tab_tab_helper.h:36: __weak id<SadTabTabHelperDelegate> delegate_; On 2017/04/11 15:17:00, Eugene But wrote: > Could you please move data member to the bottom (after methods). > > https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... Done, thanks! https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/web/... File ios/chrome/browser/web/sad_tab_tab_helper.mm (right): https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/web/... ios/chrome/browser/web/sad_tab_tab_helper.mm:9: #import "base/strings/sys_string_conversions.h" On 2017/04/11 15:17:00, Eugene But wrote: > s/import/include (because Objective-C code is guarded by ifdefs) > > https://google.github.io/styleguide/objcguide.xml#_import_and__include Done. Interesting too, thanks! https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/web/... ios/chrome/browser/web/sad_tab_tab_helper.mm:20: NSString* const SadTabTabHelperID = @"SadTabTabHelper"; On 2017/04/11 15:17:00, Eugene But wrote: > Do we still need this? Done. Good catch, thanks! https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/web/... ios/chrome/browser/web/sad_tab_tab_helper.mm:46: GURL lastCommittedURL = web_state()->GetLastCommittedURL(); On 2017/04/11 15:17:00, Eugene But wrote: > s/lastCommittedURL/last_committed_url (because this is C++ method) > > Same comment for sadTabView and contentView Done, thanks! https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/web/... ios/chrome/browser/web/sad_tab_tab_helper.mm:52: SadTabView* sadTabView = [[SadTabView alloc] initWithReloadHandler:^{ On 2017/04/11 15:17:00, Eugene But wrote: > s/sadTabView/sad_tab_view Done! https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/web/... File ios/chrome/browser/web/sad_tab_tab_helper_delegate.h (right): https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/web/... ios/chrome/browser/web/sad_tab_tab_helper_delegate.h:5: #ifndef tab_helper_delegate_h On 2017/04/11 08:17:39, sdefresne wrote: > This is not using the correct header guards. Please use src/tools/boilerplate.py > script to create file with the correct header guards. Done! Thanks! https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/web/... ios/chrome/browser/web/sad_tab_tab_helper_delegate.h:18: // Returns whether the tab for the tab helper is visible, allowing differences On 2017/04/11 15:17:00, Eugene But wrote: > Do you want to elaborate what |visible| means in this context (app is in active > state and tab is the frontmost)? Done! https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/web/... File ios/chrome/browser/web/sad_tab_tab_helper_unittest.mm (right): https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/web/... ios/chrome/browser/web/sad_tab_tab_helper_unittest.mm:22: bool DidShowTransientContentView() { return didShowTransientContentView; } On 2017/04/11 15:17:00, Eugene But wrote: > How about > > |bool is_showing_transient_content_view() const { return > is_showing_transient_content_view_; }|? Done. https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/web/... ios/chrome/browser/web/sad_tab_tab_helper_unittest.mm:25: bool didShowTransientContentView; On 2017/04/11 15:17:00, Eugene But wrote: > How about: > bool is_showing_transient_content_view_ = true; ? Done. https://codereview.chromium.org/2807843002/diff/40001/ios/chrome/browser/web/... ios/chrome/browser/web/sad_tab_tab_helper_unittest.mm:44: SadTabTabHelper::CreateForWebState(&web_state, delegate); On 2017/04/11 15:17:00, Eugene But wrote: > Optional nit: Do you want to create SadTabTabHelperTest test fixture? > > Like this: > > class SadTabTabHelperTest : public PlatformTest { > protected: > SadTabTabHelperTest() : delegate_([[TabHelperTestDelegate alloc] init]) { > SadTabTabHelper::CreateForWebState(&web_state, delegate); > } > TabHelperTestDelegate* delegate_; > SadTabTabHelperTestWebState web_state_; > }; Done, this is great, thanks!
Thank you for the patience. lgtm! Please take a look at the first comment, in our codereview tool it is very easy to miss them, because comments remain in the previous patches. https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/sad_... File ios/chrome/browser/web/sad_tab_tab_helper_unittest.mm (right): https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/sad_... ios/chrome/browser/web/sad_tab_tab_helper_unittest.mm:17: void ShowTransientContentView(CRWContentView* content_view) override { On 2017/04/11 15:17:00, Eugene But wrote: > On 2017/04/11 01:35:21, PL wrote: > > On 2017/04/10 22:56:28, Eugene But wrote: > > > Do you want to update web::TestWebState instead of adding API here? > > > > I could do that no problem, but here's why I did this: it's not possible to > > update TestWebState to do the right thing here without adding some flag (as it > > just calls through to a CRWebController so no state change in the WebState is > > currently triggered). > Are you sure that web::TestWebState uses CRWWebController? > > > I didn't see other cases of flags being added like this to the main > TestWebState > > so I created this subclass just for the purpose of this test rather than > > complicate the superclass for everyone. I saw this pattern used in other > > places such as reading_list_web_state_observer_unittest.mm. > > > > What do you think about keeping this here? > This code could be useful in other places. I think you missed this comment. https://codereview.chromium.org/2807843002/diff/60001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/sad_tab/sad_tab_view.mm (right): https://codereview.chromium.org/2807843002/diff/60001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/sad_tab/sad_tab_view.mm:57: @property(nonatomic, readonly) ProceduralBlock reloadHandler; @property(nonatomic, readonly, copy) ProceduralBlock reloadHandler; If I understand correctly |strong| is default even for blocks and that may lead to very unpleasant bugs. But maybe I'm wrong and copy is default for blocks, then this code is fine :) https://codereview.chromium.org/2807843002/diff/60001/ios/chrome/browser/web/... File ios/chrome/browser/web/sad_tab_tab_helper.mm (right): https://codereview.chromium.org/2807843002/diff/60001/ios/chrome/browser/web/... ios/chrome/browser/web/sad_tab_tab_helper.mm:40: GURL last_committed_url = web_state()->GetLastCommittedURL(); Optional nil: do you want to drop this variable? https://codereview.chromium.org/2807843002/diff/60001/ios/chrome/browser/web/... ios/chrome/browser/web/sad_tab_tab_helper.mm:45: void SadTabTabHelper::PresentSadTab(const GURL& url_causing_failure) { Sorry for not mentioning previously. Do you need |url_causing_failure| for the future CL? https://codereview.chromium.org/2807843002/diff/60001/ios/chrome/browser/web/... File ios/chrome/browser/web/sad_tab_tab_helper_delegate.h (right): https://codereview.chromium.org/2807843002/diff/60001/ios/chrome/browser/web/... ios/chrome/browser/web/sad_tab_tab_helper_delegate.h:5: #ifndef __IOS_CHROME_BROWSER_WEB_SAD_TAB_TAB_HELPER_DELEGATE_H_ IOS_CHROME_BROWSER_WEB_SAD_TAB_TAB_HELPER_DELEGATE_H_ no need for trailing underscores. |tools/boilerplate.py <relative path to source file>| creates a header with correct guards automatically.
lgtm with Eugene's comments
Thanks for this awesome review! I've made two main tweaks based on the feedback: 1. Moved my test code into TestWebState as suggested by Eugene; if you're happy having this live there, I'm happy for it to go there! 2. Removed the recording of the GURL in the helper, this will be required in a forthcoming CL, but is cleaner to omit here as it's not used. Thank you for your ongoing guidance! I'll upload a tweaked version now. https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/sad_... File ios/chrome/browser/web/sad_tab_tab_helper_unittest.mm (right): https://codereview.chromium.org/2807843002/diff/1/ios/chrome/browser/web/sad_... ios/chrome/browser/web/sad_tab_tab_helper_unittest.mm:17: void ShowTransientContentView(CRWContentView* content_view) override { On 2017/04/12 01:01:31, Eugene But wrote: > On 2017/04/11 15:17:00, Eugene But wrote: > > On 2017/04/11 01:35:21, PL wrote: > > > On 2017/04/10 22:56:28, Eugene But wrote: > > > > Do you want to update web::TestWebState instead of adding API here? > > > > > > I could do that no problem, but here's why I did this: it's not possible to > > > update TestWebState to do the right thing here without adding some flag (as > it > > > just calls through to a CRWebController so no state change in the WebState > is > > > currently triggered). > > Are you sure that web::TestWebState uses CRWWebController? > > > > > I didn't see other cases of flags being added like this to the main > > TestWebState > > > so I created this subclass just for the purpose of this test rather than > > > complicate the superclass for everyone. I saw this pattern used in other > > > places such as reading_list_web_state_observer_unittest.mm. > > > > > > What do you think about keeping this here? > > This code could be useful in other places. > I think you missed this comment. I did, thanks! I understand this a bit better now - you're right that TestWebState does *not* use CRWWebController, rather I was looking at the real implementation. I understand the TestWebState a little better now (at first I was mistakenly assuming it was a subclass of the real WebStateImpl), so I can add this to the TestWebState easily. Thanks! https://codereview.chromium.org/2807843002/diff/60001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/sad_tab/sad_tab_view.mm (right): https://codereview.chromium.org/2807843002/diff/60001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/sad_tab/sad_tab_view.mm:57: @property(nonatomic, readonly) ProceduralBlock reloadHandler; On 2017/04/12 01:01:31, Eugene But wrote: > @property(nonatomic, readonly, copy) ProceduralBlock reloadHandler; > If I understand correctly |strong| is default even for blocks and that may lead > to very unpleasant bugs. But maybe I'm wrong and copy is default for blocks, > then this code is fine :) Copy and Strong don't cause any difference to a block property under ARC, so this should work correctly. There are some places (some Apple docs for example) that recommend using the property type which you 'know' is going to be applied by the compiler (in this case, a copy) - but I'd like to reject this as I think it indicates control that is not present. Given this, are you happy to leave this as is? https://codereview.chromium.org/2807843002/diff/60001/ios/chrome/browser/web/... File ios/chrome/browser/web/sad_tab_tab_helper.mm (right): https://codereview.chromium.org/2807843002/diff/60001/ios/chrome/browser/web/... ios/chrome/browser/web/sad_tab_tab_helper.mm:40: GURL last_committed_url = web_state()->GetLastCommittedURL(); On 2017/04/12 01:01:32, Eugene But wrote: > Optional nil: do you want to drop this variable? Good point let me address this below: https://codereview.chromium.org/2807843002/diff/60001/ios/chrome/browser/web/... ios/chrome/browser/web/sad_tab_tab_helper.mm:45: void SadTabTabHelper::PresentSadTab(const GURL& url_causing_failure) { On 2017/04/12 01:01:31, Eugene But wrote: > Sorry for not mentioning previously. Do you need |url_causing_failure| for the > future CL? This is something I'll need for the second phase of this work. But as it's not used in this CL I'll take it out and just include it in the future CL. Thanks! https://codereview.chromium.org/2807843002/diff/60001/ios/chrome/browser/web/... File ios/chrome/browser/web/sad_tab_tab_helper_delegate.h (right): https://codereview.chromium.org/2807843002/diff/60001/ios/chrome/browser/web/... ios/chrome/browser/web/sad_tab_tab_helper_delegate.h:5: #ifndef __IOS_CHROME_BROWSER_WEB_SAD_TAB_TAB_HELPER_DELEGATE_H_ On 2017/04/12 01:01:32, Eugene But wrote: > IOS_CHROME_BROWSER_WEB_SAD_TAB_TAB_HELPER_DELEGATE_H_ no need for trailing > underscores. > > |tools/boilerplate.py <relative path to source file>| creates a header with > correct guards automatically. This is the boilerplate boilerplate.py generated, I think I must have used it wrong! I've removed the prefixed underscores, thanks!
still lgtm! https://codereview.chromium.org/2807843002/diff/60001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/sad_tab/sad_tab_view.mm (right): https://codereview.chromium.org/2807843002/diff/60001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/sad_tab/sad_tab_view.mm:57: @property(nonatomic, readonly) ProceduralBlock reloadHandler; On 2017/04/12 17:07:38, peterlaurens wrote: > On 2017/04/12 01:01:31, Eugene But wrote: > > @property(nonatomic, readonly, copy) ProceduralBlock reloadHandler; > > If I understand correctly |strong| is default even for blocks and that may > lead > > to very unpleasant bugs. But maybe I'm wrong and copy is default for blocks, > > then this code is fine :) > > Copy and Strong don't cause any difference to a block property under ARC, so > this should work correctly. > > There are some places (some Apple docs for example) that recommend using the > property type which you 'know' is going to be applied by the compiler (in this > case, a copy) - but I'd like to reject this as I think it indicates control that > is not present. > > Given this, are you happy to leave this as is? Thanks for explanation. I'm happy with the current code. https://codereview.chromium.org/2807843002/diff/80001/ios/web/public/test/fak... File ios/web/public/test/fakes/test_web_state.mm (right): https://codereview.chromium.org/2807843002/diff/80001/ios/web/public/test/fak... ios/web/public/test/fakes/test_web_state.mm:187: is_showing_transient_content_view_ = true; Do you want to set this to false in ClearTransientContentView?
ClearTransientContentView added, thanks! https://codereview.chromium.org/2807843002/diff/80001/ios/web/public/test/fak... File ios/web/public/test/fakes/test_web_state.mm (right): https://codereview.chromium.org/2807843002/diff/80001/ios/web/public/test/fak... ios/web/public/test/fakes/test_web_state.mm:187: is_showing_transient_content_view_ = true; On 2017/04/12 17:40:08, Eugene But wrote: > Do you want to set this to false in ClearTransientContentView? Yes! Great idea, thanks!
The CQ bit was checked by peterlaurens@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kkhorimoto@chromium.org, eugenebut@chromium.org Link to the patchset: https://codereview.chromium.org/2807843002/#ps100001 (title: "Adding ClearTransientContentView to TestWebState")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by peterlaurens@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by peterlaurens@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kkhorimoto@chromium.org, eugenebut@chromium.org Link to the patchset: https://codereview.chromium.org/2807843002/#ps120001 (title: "Attempt to fix build dependency")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1492049512019380,
"parent_rev": "998bf426e310eda213fa80898a840a89f815d676", "commit_rev":
"5f9ee9e0a99624061377745d7b8261c5a9bc59b1"}
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1492049512019380,
"parent_rev": "c00b9e07d88486e3f15226b7e37be732e57ec897", "commit_rev":
"ace0d4cd1d5616bd8ea03ce8ed314857c15d4b3a"}
Message was sent while issue was closed.
Description was changed from ========== Refactor creation of SadTabView into a tab helper object and reduce dependence on Tab. BUG=Sad Tab works as expected ========== to ========== Refactor creation of SadTabView into a tab helper object and reduce dependence on Tab. BUG=Sad Tab works as expected Review-Url: https://codereview.chromium.org/2807843002 Cr-Commit-Position: refs/heads/master@{#464269} Committed: https://chromium.googlesource.com/chromium/src/+/ace0d4cd1d5616bd8ea03ce8ed31... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/ace0d4cd1d5616bd8ea03ce8ed31... |
