Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2589213002:
Add CRWNativeContent willBeDismissed. (Closed)
DescriptionAdd CRWNativeContent willBeDismissed.
It is usefull for a native content to be warned when it will be
dismissed to do some cleanup.
Add an optional method in the protocol.
BUG=671160
Committed: https://crrev.com/a35ed7503fff85852efdb468c218d4f24a2ba489
Cr-Commit-Position: refs/heads/master@{#440354}
Patch Set 1 #Patch Set 2 : blank line #Patch Set 3 : call in setNativeController #Patch Set 4 : in container view #
Total comments: 8
Patch Set 5 : feedback #
Dependent Patchsets: Messages
Total messages: 19 (5 generated)
olivierrobin@chromium.org changed reviewers: + eugenebut@chromium.org, kkhorimoto@chromium.org
A couple high level questions: 1) Why is |-willBeDismissed| necessary when we could perform cleanup in |-dealloc| or |-close|? I know that we've had some issues with leaking native controllers, so maybe |-dealloc| won't work. 2) I don't agree with the |-clearTransientContentView| => |-clearNativeView| change. Transient content views are used for interstitials and sad tabs, neither of which are native controllers. Moreover, in your current implementation, |-clearNativeView| wouldn't even clear a native controller's view, as it would early return when there's no transient content view. A better place to add the |-willBeDismissed| call would be in CRWWebControllerContainerView's |-setNativeController:| before removing the subview.
> 2) I don't agree with the |-clearTransientContentView| => |-clearNativeView| > change. Transient content views are used for interstitials and sad tabs, > neither of which are native controllers. Moreover, in your current > implementation, |-clearNativeView| wouldn't even clear a native controller's > view, as it would early return when there's no transient content view. A better > place to add the |-willBeDismissed| call would be in > CRWWebControllerContainerView's |-setNativeController:| before removing the > subview. Sorry, I suggested to call |willBeDismissed| inside |clearTransientContentView| (and rename it), to make code more maintainable. Kurt would it be ok with you to use more generic name for |clearNativeView|? Although calling |willBeDismissed| inside |setNativeController| sounds like a much better suggestion.
On 2016/12/20 19:29:33, Eugene But wrote: > > 2) I don't agree with the |-clearTransientContentView| => |-clearNativeView| > > change. Transient content views are used for interstitials and sad tabs, > > neither of which are native controllers. Moreover, in your current > > implementation, |-clearNativeView| wouldn't even clear a native controller's > > view, as it would early return when there's no transient content view. A > better > > place to add the |-willBeDismissed| call would be in > > CRWWebControllerContainerView's |-setNativeController:| before removing the > > subview. > Sorry, I suggested to call |willBeDismissed| inside |clearTransientContentView| > (and rename it), to make code more maintainable. Kurt would it be ok with you to > use more generic name for |clearNativeView|? Although calling |willBeDismissed| > inside |setNativeController| sounds like a much better suggestion. I prefer using the more specific name of clearTransientContentView since the concept of "transient content views" is already present in the WebState and CRWWebControllerContainerView APIs. The commenting for these interfaces make it clear that it's a scrollable view that is displayed temporarily until a navigation occurs. Changing it to "native view" is confusing because it's too similar to the views vended by CRWNativeContent objects, which is an entirely separate concept.
On 2016/12/20 19:36:51, kkhorimoto_ wrote: > On 2016/12/20 19:29:33, Eugene But wrote: > > > 2) I don't agree with the |-clearTransientContentView| => |-clearNativeView| > > > change. Transient content views are used for interstitials and sad tabs, > > > neither of which are native controllers. Moreover, in your current > > > implementation, |-clearNativeView| wouldn't even clear a native controller's > > > view, as it would early return when there's no transient content view. A > > better > > > place to add the |-willBeDismissed| call would be in > > > CRWWebControllerContainerView's |-setNativeController:| before removing the > > > subview. > > Sorry, I suggested to call |willBeDismissed| inside > |clearTransientContentView| > > (and rename it), to make code more maintainable. Kurt would it be ok with you > to > > use more generic name for |clearNativeView|? Although calling > |willBeDismissed| > > inside |setNativeController| sounds like a much better suggestion. > > I prefer using the more specific name of clearTransientContentView since the > concept of "transient content views" is already present in the WebState and > CRWWebControllerContainerView APIs. The commenting for these interfaces make it > clear that it's a scrollable view that is displayed temporarily until a > navigation occurs. Changing it to "native view" is confusing because it's too > similar to the views vended by CRWNativeContent objects, which is an entirely > separate concept. close is called on closing a tab, so not on all navigation. dealloc seems to be called at the correct moment right now. But I don't want to use it, because I want to be called before the next navigation, and with ARC, I am not sure this will be guaranteed. I think I could call willBeDismissed from setNativeController. Would that seems acceptable? As I want to restore the lastCommitedEntry, I need a method called before the next navigation.
On 2016/12/20 21:24:05, Olivier Robin wrote: > On 2016/12/20 19:36:51, kkhorimoto_ wrote: > > On 2016/12/20 19:29:33, Eugene But wrote: > > > > 2) I don't agree with the |-clearTransientContentView| => > |-clearNativeView| > > > > change. Transient content views are used for interstitials and sad tabs, > > > > neither of which are native controllers. Moreover, in your current > > > > implementation, |-clearNativeView| wouldn't even clear a native > controller's > > > > view, as it would early return when there's no transient content view. A > > > better > > > > place to add the |-willBeDismissed| call would be in > > > > CRWWebControllerContainerView's |-setNativeController:| before removing > the > > > > subview. > > > Sorry, I suggested to call |willBeDismissed| inside > > |clearTransientContentView| > > > (and rename it), to make code more maintainable. Kurt would it be ok with > you > > to > > > use more generic name for |clearNativeView|? Although calling > > |willBeDismissed| > > > inside |setNativeController| sounds like a much better suggestion. > > > > I prefer using the more specific name of clearTransientContentView since the > > concept of "transient content views" is already present in the WebState and > > CRWWebControllerContainerView APIs. The commenting for these interfaces make > it > > clear that it's a scrollable view that is displayed temporarily until a > > navigation occurs. Changing it to "native view" is confusing because it's too > > similar to the views vended by CRWNativeContent objects, which is an entirely > > separate concept. > > close is called on closing a tab, so not on all navigation. > dealloc seems to be called at the correct moment right now. But I don't want to > use it, because I want to be called before the > next navigation, and with ARC, I am not sure this will be guaranteed. > I think I could call willBeDismissed from setNativeController. > Would that seems acceptable? > As I want to restore the lastCommitedEntry, I need a method called before the > next navigation. Yes, I think that calling it from |-setNativeController:| and not renaming |-clearTransientContentView| would be the best approach here.
Done. Thanks
https://codereview.chromium.org/2589213002/diff/60001/ios/web/public/web_stat... File ios/web/public/web_state/ui/crw_native_content.h (right): https://codereview.chromium.org/2589213002/diff/60001/ios/web/public/web_stat... ios/web/public/web_state/ui/crw_native_content.h:88: // Called when a navigation is triggered from this nativeContent. nit: Is this called on reload? Should this comment be "Notifies the CRWNativeContent that it will be removed from superview"? https://codereview.chromium.org/2589213002/diff/60001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller_container_view.mm (right): https://codereview.chromium.org/2589213002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller_container_view.mm:173: if ([_nativeController respondsToSelector:@selector(willBeDismissed)]) { Should this be moved inside |(![_nativeController isEqual:nativeController])| check? Otherwise |willBeDismissed| may be called even for cases when nativeController is not changed, which does not sound right to me.
https://codereview.chromium.org/2589213002/diff/60001/ios/web/public/web_stat... File ios/web/public/web_state/ui/crw_native_content.h (right): https://codereview.chromium.org/2589213002/diff/60001/ios/web/public/web_stat... ios/web/public/web_state/ui/crw_native_content.h:88: // Called when a navigation is triggered from this nativeContent. On 2016/12/21 15:26:35, Eugene But wrote: > nit: Is this called on reload? Should this comment be "Notifies the > CRWNativeContent that it will be removed from superview"? It is called on reload. I will change the comment. https://codereview.chromium.org/2589213002/diff/60001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller_container_view.mm (right): https://codereview.chromium.org/2589213002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller_container_view.mm:173: if ([_nativeController respondsToSelector:@selector(willBeDismissed)]) { On 2016/12/21 15:26:35, Eugene But wrote: > Should this be moved inside |(![_nativeController isEqual:nativeController])| > check? Otherwise |willBeDismissed| may be called even for cases when > nativeController is not changed, which does not sound right to me. The logic is to call in any navigation. If the nativeContentProvider gives the same nativeController for two navigations, it should be called twice. That is why I put it here. I don't think it change a lot for OfflinePageNativeContent as BVC creates a new one each time, so I can put it inside.
https://codereview.chromium.org/2589213002/diff/60001/ios/web/public/web_stat... File ios/web/public/web_state/ui/crw_native_content.h (right): https://codereview.chromium.org/2589213002/diff/60001/ios/web/public/web_stat... ios/web/public/web_state/ui/crw_native_content.h:88: // Called when a navigation is triggered from this nativeContent. On 2016/12/21 15:41:11, Olivier Robin wrote: > On 2016/12/21 15:26:35, Eugene But wrote: > > nit: Is this called on reload? Should this comment be "Notifies the > > CRWNativeContent that it will be removed from superview"? > > It is called on reload. > I will change the comment. Actually it is not called on reload. https://codereview.chromium.org/2589213002/diff/60001/ios/web/public/web_stat... ios/web/public/web_state/ui/crw_native_content.h:88: // Called when a navigation is triggered from this nativeContent. On 2016/12/21 15:26:35, Eugene But wrote: > nit: Is this called on reload? Should this comment be "Notifies the > CRWNativeContent that it will be removed from superview"? Done. https://codereview.chromium.org/2589213002/diff/60001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller_container_view.mm (right): https://codereview.chromium.org/2589213002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller_container_view.mm:173: if ([_nativeController respondsToSelector:@selector(willBeDismissed)]) { On 2016/12/21 15:26:35, Eugene But wrote: > Should this be moved inside |(![_nativeController isEqual:nativeController])| > check? Otherwise |willBeDismissed| may be called even for cases when > nativeController is not changed, which does not sound right to me. Done.
lgtm https://codereview.chromium.org/2589213002/diff/60001/ios/web/public/web_stat... File ios/web/public/web_state/ui/crw_native_content.h (right): https://codereview.chromium.org/2589213002/diff/60001/ios/web/public/web_stat... ios/web/public/web_state/ui/crw_native_content.h:88: // Called when a navigation is triggered from this nativeContent. On 2016/12/21 15:51:36, Olivier Robin wrote: > On 2016/12/21 15:41:11, Olivier Robin wrote: > > On 2016/12/21 15:26:35, Eugene But wrote: > > > nit: Is this called on reload? Should this comment be "Notifies the > > > CRWNativeContent that it will be removed from superview"? > > > > It is called on reload. > > I will change the comment. > > Actually it is not called on reload. With your new comment no one will expect this to be called on reload, so this is fine :)
The CQ bit was checked by olivierrobin@chromium.org
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": 80001, "attempt_start_ts": 1482391882481170,
"parent_rev": "c91d9b34146c0058e9e130b7c515d357120560cb", "commit_rev":
"48e4b2b7d346b733e65950bbd68cf5410b9e4495"}
Message was sent while issue was closed.
Description was changed from ========== Add CRWNativeContent willBeDismissed. It is usefull for a native content to be warned when it will be dismissed to do some cleanup. Add an optional method in the protocol. BUG=671160 ========== to ========== Add CRWNativeContent willBeDismissed. It is usefull for a native content to be warned when it will be dismissed to do some cleanup. Add an optional method in the protocol. BUG=671160 Review-Url: https://codereview.chromium.org/2589213002 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Add CRWNativeContent willBeDismissed. It is usefull for a native content to be warned when it will be dismissed to do some cleanup. Add an optional method in the protocol. BUG=671160 Review-Url: https://codereview.chromium.org/2589213002 ========== to ========== Add CRWNativeContent willBeDismissed. It is usefull for a native content to be warned when it will be dismissed to do some cleanup. Add an optional method in the protocol. BUG=671160 Committed: https://crrev.com/a35ed7503fff85852efdb468c218d4f24a2ba489 Cr-Commit-Position: refs/heads/master@{#440354} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/a35ed7503fff85852efdb468c218d4f24a2ba489 Cr-Commit-Position: refs/heads/master@{#440354} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
