Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2627093003:
Reuse context menu in StaticHTMLViewController (Closed)
|
Created:
3 years, 11 months ago by Olivier Modified:
3 years, 11 months ago CC:
chromium-reviews, pkl (ping after 24h if needed), jam, noyau+watch_chromium.org, darin-cc_chromium.org, marq+watch_chromium.org, stkhapugin, sdefresne+watch_chromium.org, noyau (Ping after 24h) Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReuse context menu in StaticHTMLViewController
Create a CRWContextMenuController using the logic from CRWWebController.
Reuse this class for StaticHTMLViewController.
BUG=679818, 679793
Review-Url: https://codereview.chromium.org/2627093003
Cr-Commit-Position: refs/heads/master@{#443861}
Committed: https://chromium.googlesource.com/chromium/src/+/d9ca613db4a8616806d3447db5651870ed7c43fe
Patch Set 1 #Patch Set 2 : clean #Patch Set 3 : fix DEPS #
Total comments: 35
Patch Set 4 : cleaning #
Total comments: 34
Patch Set 5 : feedback #
Total comments: 27
Patch Set 6 : other feedback #
Total comments: 2
Patch Set 7 : feedback #Messages
Total messages: 31 (8 generated)
olivierrobin@chromium.org changed reviewers: + eugenebut@chromium.org, jif@chromium.org
Hi eugenebut, I am not sure the place I put the new class is the best. If there is a better place/name to put it, please let me know.
On 2017/01/11 18:11:50, Olivier Robin wrote: > Hi eugenebut, > > I am not sure the place I put the new class is the best. > If there is a better place/name to put it, please let me know. Generally I love extracting Context Menu code from WebController. But now I'm worrying that offline_page_native_content may not be the right place for Offline Pages. So now we have to support ContextMenu in 2 places. Later we may have to support Force Touch in both places. And in a while who knows which features will have to be duplicated. CRWWebController supports loading plain HTML, and because ios/web does not support net stack, maybe we should add API for loading HTML to WebState?
On 2017/01/11 19:57:20, Eugene But wrote: > On 2017/01/11 18:11:50, Olivier Robin wrote: > > Hi eugenebut, > > > > I am not sure the place I put the new class is the best. > > If there is a better place/name to put it, please let me know. > Generally I love extracting Context Menu code from WebController. But now I'm > worrying that offline_page_native_content may not be the right place for Offline > Pages. > > So now we have to support ContextMenu in 2 places. Later we may have to support > Force Touch in both places. And in a while who knows which features will have to > be duplicated. > CRWWebController supports loading plain HTML, and because ios/web does not > support net stack, maybe we should add API for loading HTML to WebState? CRWWebController does not support loading files. Using loadHTML would require inlining everything. We may want to change the way offline pages, but this will require changing more things (specially the virtualURL/URL logic) that I would prefer not change 1 week before branch. If we make the 3D touch generic enough, we will be able to reuse it for staticHTML too.
On 2017/01/11 20:02:16, Olivier Robin wrote: > On 2017/01/11 19:57:20, Eugene But wrote: > > On 2017/01/11 18:11:50, Olivier Robin wrote: > > > Hi eugenebut, > > > > > > I am not sure the place I put the new class is the best. > > > If there is a better place/name to put it, please let me know. > > Generally I love extracting Context Menu code from WebController. But now I'm > > worrying that offline_page_native_content may not be the right place for > Offline > > Pages. > > > > So now we have to support ContextMenu in 2 places. Later we may have to > support > > Force Touch in both places. And in a while who knows which features will have > to > > be duplicated. > > CRWWebController supports loading plain HTML, and because ios/web does not > > support net stack, maybe we should add API for loading HTML to WebState? > > CRWWebController does not support loading files. Using loadHTML would require > inlining everything. We could add this API to workaround net stack absence. In this case diverging from content seems to be a reasonable approach. > We may want to change the way offline pages, but this will require changing more > things (specially the > virtualURL/URL logic) that I would prefer not change 1 week before branch. We should aim to do the right thing and ship features when they ready to ship, so I don't think that time sensitivity is a good argument for that. > If we make the 3D touch generic enough, we will be able to reuse it for > staticHTML too. But do we really want to have 2 separate classes to display online and offline content if those classes need the same features? If Offline Content page needs features already supported in web then would it make sense to make ios/web more flexible and reuse it? Can you think about any reasons why reusing ios/web for Offline Pages is not appropriate? If you still want to have separate classes, then you can do that, but then we need to find a better place for crw_context_menu_controller.h. The purpose of ios/web/public directory is to have web public API and if we want to reuse ui code, then //ui layer sounds more appropriate to me.
On 2017/01/11 21:28:23, Eugene But wrote: > On 2017/01/11 20:02:16, Olivier Robin wrote: > > On 2017/01/11 19:57:20, Eugene But wrote: > > > On 2017/01/11 18:11:50, Olivier Robin wrote: > > > > Hi eugenebut, > > > > > > > > I am not sure the place I put the new class is the best. > > > > If there is a better place/name to put it, please let me know. > > > Generally I love extracting Context Menu code from WebController. But now > I'm > > > worrying that offline_page_native_content may not be the right place for > > Offline > > > Pages. > > > > > > So now we have to support ContextMenu in 2 places. Later we may have to > > support > > > Force Touch in both places. And in a while who knows which features will > have > > to > > > be duplicated. > > > CRWWebController supports loading plain HTML, and because ios/web does not > > > support net stack, maybe we should add API for loading HTML to WebState? > > > > CRWWebController does not support loading files. Using loadHTML would require > > inlining everything. > We could add this API to workaround net stack absence. In this case diverging > from content seems to be a reasonable approach. > > > We may want to change the way offline pages, but this will require changing > more > > things (specially the > > virtualURL/URL logic) that I would prefer not change 1 week before branch. > We should aim to do the right thing and ship features when they ready to ship, > so I don't think that time sensitivity is a good argument for that. > > > If we make the 3D touch generic enough, we will be able to reuse it for > > staticHTML too. > But do we really want to have 2 separate classes to display online and offline > content if those classes need the same features? > If Offline Content page needs features already supported in web then would it > make sense to make ios/web more flexible and reuse it? > Can you think about any reasons why reusing ios/web for Offline Pages is not > appropriate? > > If you still want to have separate classes, then you can do that, but then we > need to find a better place for crw_context_menu_controller.h. > The purpose of ios/web/public directory is to have web public API and if we want > to reuse ui code, then //ui layer sounds more appropriate to me. Oh, wait... ContextMenuController can't leave outside of web because it calls JS web functions. Also chrome should not use ContextMenuController because context menu controller expect certain JS functions to be injected and those functions are private implementation details of web. Can we have a doc where we will discuss what features we expect in offline mode and whether or not we can use OfflinePageNativeContent for that?
On 2017/01/11 21:50:16, Eugene But wrote: > On 2017/01/11 21:28:23, Eugene But wrote: > > On 2017/01/11 20:02:16, Olivier Robin wrote: > > > On 2017/01/11 19:57:20, Eugene But wrote: > > > > On 2017/01/11 18:11:50, Olivier Robin wrote: > > > > > Hi eugenebut, > > > > > > > > > > I am not sure the place I put the new class is the best. > > > > > If there is a better place/name to put it, please let me know. > > > > Generally I love extracting Context Menu code from WebController. But now > > I'm > > > > worrying that offline_page_native_content may not be the right place for > > > Offline > > > > Pages. > > > > > > > > So now we have to support ContextMenu in 2 places. Later we may have to > > > support > > > > Force Touch in both places. And in a while who knows which features will > > have > > > to > > > > be duplicated. > > > > CRWWebController supports loading plain HTML, and because ios/web does not > > > > support net stack, maybe we should add API for loading HTML to WebState? > > > > > > CRWWebController does not support loading files. Using loadHTML would > require > > > inlining everything. > > We could add this API to workaround net stack absence. In this case diverging > > from content seems to be a reasonable approach. > > > > > We may want to change the way offline pages, but this will require changing > > more > > > things (specially the > > > virtualURL/URL logic) that I would prefer not change 1 week before branch. > > We should aim to do the right thing and ship features when they ready to ship, > > so I don't think that time sensitivity is a good argument for that. > > > > > If we make the 3D touch generic enough, we will be able to reuse it for > > > staticHTML too. > > But do we really want to have 2 separate classes to display online and offline > > content if those classes need the same features? > > If Offline Content page needs features already supported in web then would it > > make sense to make ios/web more flexible and reuse it? > > Can you think about any reasons why reusing ios/web for Offline Pages is not > > appropriate? > > > > If you still want to have separate classes, then you can do that, but then we > > need to find a better place for crw_context_menu_controller.h. > > The purpose of ios/web/public directory is to have web public API and if we > want > > to reuse ui code, then //ui layer sounds more appropriate to me. > Oh, wait... ContextMenuController can't leave outside of web because it calls JS > web functions. Also chrome should not use ContextMenuController because context > menu controller expect certain JS functions to be injected and those functions > are private implementation details of web. > > Can we have a doc where we will discuss what features we expect in offline mode > and whether or not we can use OfflinePageNativeContent for that? After giving some thoughts it looks like allowing WebState to load files would be significant engineering effort. And I'm not sure if it worth doing. Creating a class which allows hijacking context menu for WKWebView is probably the only viable approach which you did in this CL. However if you want to follow this path and create ContextMenuController, then please make it independent from ios/web and do not call __gCrWeb functions. Also this ContextMenuController probably belongs to ui layer, but this is something that you will have to confirm with ui owners. In any case, this looks like a non trivial change, so please create a design doc. Thanks!
Description was changed from ========== Reuse context menu in StaticHTMLViewController Create a CRWContextMenuController using the logic from CRWWebController. Reuse this class for StaticHTMLViewController. BUG=679818, 679793 ========== to ========== Reuse context menu in StaticHTMLViewController Create a CRWContextMenuController using the logic from CRWWebController. Reuse this class for StaticHTMLViewController. BUG=679818, 679793 ==========
On 2017/01/12 01:26:28, Eugene But wrote: > On 2017/01/11 21:50:16, Eugene But wrote: > > On 2017/01/11 21:28:23, Eugene But wrote: > > > On 2017/01/11 20:02:16, Olivier Robin wrote: > > > > On 2017/01/11 19:57:20, Eugene But wrote: > > > > > On 2017/01/11 18:11:50, Olivier Robin wrote: > > > > > > Hi eugenebut, > > > > > > > > > > > > I am not sure the place I put the new class is the best. > > > > > > If there is a better place/name to put it, please let me know. > > > > > Generally I love extracting Context Menu code from WebController. But > now > > > I'm > > > > > worrying that offline_page_native_content may not be the right place for > > > > Offline > > > > > Pages. > > > > > > > > > > So now we have to support ContextMenu in 2 places. Later we may have to > > > > support > > > > > Force Touch in both places. And in a while who knows which features will > > > have > > > > to > > > > > be duplicated. > > > > > CRWWebController supports loading plain HTML, and because ios/web does > not > > > > > support net stack, maybe we should add API for loading HTML to WebState? > > > > > > > > CRWWebController does not support loading files. Using loadHTML would > > require > > > > inlining everything. > > > We could add this API to workaround net stack absence. In this case > diverging > > > from content seems to be a reasonable approach. > > > > > > > We may want to change the way offline pages, but this will require > changing > > > more > > > > things (specially the > > > > virtualURL/URL logic) that I would prefer not change 1 week before branch. > > > We should aim to do the right thing and ship features when they ready to > ship, > > > so I don't think that time sensitivity is a good argument for that. > > > > > > > If we make the 3D touch generic enough, we will be able to reuse it for > > > > staticHTML too. > > > But do we really want to have 2 separate classes to display online and > offline > > > content if those classes need the same features? > > > If Offline Content page needs features already supported in web then would > it > > > make sense to make ios/web more flexible and reuse it? > > > Can you think about any reasons why reusing ios/web for Offline Pages is not > > > appropriate? > > > > > > If you still want to have separate classes, then you can do that, but then > we > > > need to find a better place for crw_context_menu_controller.h. > > > The purpose of ios/web/public directory is to have web public API and if we > > want > > > to reuse ui code, then //ui layer sounds more appropriate to me. > > Oh, wait... ContextMenuController can't leave outside of web because it calls > JS > > web functions. Also chrome should not use ContextMenuController because > context > > menu controller expect certain JS functions to be injected and those functions > > are private implementation details of web. > > > > Can we have a doc where we will discuss what features we expect in offline > mode > > and whether or not we can use OfflinePageNativeContent for that? > After giving some thoughts it looks like allowing WebState to load files would > be significant engineering effort. And I'm not sure if it worth doing. > > Creating a class which allows hijacking context menu for WKWebView is probably > the only viable approach which you did in this CL. > However if you want to follow this path and create ContextMenuController, then > please make it independent from ios/web and do not call __gCrWeb functions. > Also this ContextMenuController probably belongs to ui layer, but this is > something that you will have to confirm with ui owners. In any case, this looks > like a non trivial change, so please create a design doc. > > Thanks! Hi, I am fixing a simple bug of reading list. I thought that reuse the code of ios/web was a good idea. Reading list will be in M57 and I don't think refactoring is a good reason for postponing a feature. The WKWebView are created in ios/web BuildWKWebView. The script are injected in ios/web by WKWebViewConfigurationProvider. So it seems logical to me create the context menu in ios/web, exactly as it was done before, and there would be no dependency problem. We can even make this part of BuildWKWebView if this solve the dependency problem. If this is not possible, then I will find another way (duplicate needed code in ios/chrome) to fix the code, and just close this CL. I have no plan to do a design doc to move this file or reimplement the javascript (especially getElementFromPoint).
On 2017/01/12 08:25:54, Olivier Robin wrote: > On 2017/01/12 01:26:28, Eugene But wrote: > > On 2017/01/11 21:50:16, Eugene But wrote: > > > On 2017/01/11 21:28:23, Eugene But wrote: > > > > On 2017/01/11 20:02:16, Olivier Robin wrote: > > > > > On 2017/01/11 19:57:20, Eugene But wrote: > > > > > > On 2017/01/11 18:11:50, Olivier Robin wrote: > > > > > > > Hi eugenebut, > > > > > > > > > > > > > > I am not sure the place I put the new class is the best. > > > > > > > If there is a better place/name to put it, please let me know. > > > > > > Generally I love extracting Context Menu code from WebController. But > > now > > > > I'm > > > > > > worrying that offline_page_native_content may not be the right place > for > > > > > Offline > > > > > > Pages. > > > > > > > > > > > > So now we have to support ContextMenu in 2 places. Later we may have > to > > > > > support > > > > > > Force Touch in both places. And in a while who knows which features > will > > > > have > > > > > to > > > > > > be duplicated. > > > > > > CRWWebController supports loading plain HTML, and because ios/web does > > not > > > > > > support net stack, maybe we should add API for loading HTML to > WebState? > > > > > > > > > > CRWWebController does not support loading files. Using loadHTML would > > > require > > > > > inlining everything. > > > > We could add this API to workaround net stack absence. In this case > > diverging > > > > from content seems to be a reasonable approach. > > > > > > > > > We may want to change the way offline pages, but this will require > > changing > > > > more > > > > > things (specially the > > > > > virtualURL/URL logic) that I would prefer not change 1 week before > branch. > > > > We should aim to do the right thing and ship features when they ready to > > ship, > > > > so I don't think that time sensitivity is a good argument for that. > > > > > > > > > If we make the 3D touch generic enough, we will be able to reuse it for > > > > > staticHTML too. > > > > But do we really want to have 2 separate classes to display online and > > offline > > > > content if those classes need the same features? > > > > If Offline Content page needs features already supported in web then would > > it > > > > make sense to make ios/web more flexible and reuse it? > > > > Can you think about any reasons why reusing ios/web for Offline Pages is > not > > > > appropriate? > > > > > > > > If you still want to have separate classes, then you can do that, but then > > we > > > > need to find a better place for crw_context_menu_controller.h. > > > > The purpose of ios/web/public directory is to have web public API and if > we > > > want > > > > to reuse ui code, then //ui layer sounds more appropriate to me. > > > Oh, wait... ContextMenuController can't leave outside of web because it > calls > > JS > > > web functions. Also chrome should not use ContextMenuController because > > context > > > menu controller expect certain JS functions to be injected and those > functions > > > are private implementation details of web. > > > > > > Can we have a doc where we will discuss what features we expect in offline > > mode > > > and whether or not we can use OfflinePageNativeContent for that? > > After giving some thoughts it looks like allowing WebState to load files would > > be significant engineering effort. And I'm not sure if it worth doing. > > > > Creating a class which allows hijacking context menu for WKWebView is probably > > the only viable approach which you did in this CL. > > However if you want to follow this path and create ContextMenuController, then > > please make it independent from ios/web and do not call __gCrWeb functions. > > Also this ContextMenuController probably belongs to ui layer, but this is > > something that you will have to confirm with ui owners. In any case, this > looks > > like a non trivial change, so please create a design doc. > > > > Thanks! > > Hi, > > I am fixing a simple bug of reading list. > I thought that reuse the code of ios/web was a good idea. > Reading list will be in M57 and I don't think refactoring is a good reason for > postponing a feature. > > The WKWebView are created in ios/web BuildWKWebView. > The script are injected in ios/web by WKWebViewConfigurationProvider. > So it seems logical to me create the context menu in ios/web, exactly as it was > done before, and there would be no dependency problem. > We can even make this part of BuildWKWebView if this solve the dependency > problem. > > If this is not possible, then I will find another way (duplicate needed code in > ios/chrome) to fix the code, and just close this CL. > I have no plan to do a design doc to move this file or reimplement the > javascript (especially getElementFromPoint). Codereuse of ios/web is a good idea (extracting context menu code is great for many reasons). CRWContextMenuController in its current form is problematic, because it relies on the fact that all web JS scripts are injected. Chrome layer is not suppose to know about __gCrWeb scripts and CRWContextMenuController header has no comments that would explain how to use it properly. I'm not asking to refactor everything or postpone the feature. I'm asking to address certain issues with CRWContextMenuController (for starters it is unclear for clients how to use it). Idea of using BuildWKWebView is an interesting one and can be cleanly integrated with ios/web. One simple approach could be the following: 1.) Private CRWContextMenuController class placed in ios/web/ui (one that you wrote) 2.) CRWContextMenuDelegate protocol, placed in ios/web/public (one that you wrote) 3.) A function which creates WKWebView using BuildWKWebView and attaches CRWContextMenuController as associated object placed in web_view_creation_util.h So public API for this can be as follows: @protocol CRWContextMenuDelegate - (void)webView:(WKWebView*)webView handleContextMenu:(web::ContextMenuParams&)params @end WKWebView* BuildWKWebView(CGRect frame, BrowserState* browser_state, CRWContextMenuDelegate*); Now, the reason why I'm asking for doc is that it is hard to review CLs like this: 1.) Adds ios/web public API which is not present in content 2.) Does not explain why we should diverge from content, why alternatives do not work and which alternatives we considered 3.) CL description does not explain details of the change 4.) New headers have no comments and it's unclear how to use them (new API requires certain preconditions, like web view must be created via BuildWebView).
https://codereview.chromium.org/2627093003/diff/40001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/browser_view_controller.mm (right): https://codereview.chromium.org/2627093003/diff/40001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/browser_view_controller.mm:297: ContextMenuDelegate, Does it make sense to use the CRWWebStateDelegate instead of ContextMenuDelegate? https://codereview.chromium.org/2627093003/diff/40001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/offline_page_native_content.mm (right): https://codereview.chromium.org/2627093003/diff/40001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/offline_page_native_content.mm:108: #pragma mark - CRWContextMenuControllerDelegate ? https://codereview.chromium.org/2627093003/diff/40001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/static_content/static_html_view_controller.h (right): https://codereview.chromium.org/2627093003/diff/40001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/static_content/static_html_view_controller.h:76: // loading resources and must not be null. Say that |contextMenuDelegate| can be nil. https://codereview.chromium.org/2627093003/diff/40001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/static_content/static_html_view_controller.mm (right): https://codereview.chromium.org/2627093003/diff/40001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/static_content/static_html_view_controller.mm:49: UIGestureRecognizerDelegate> { remove UIGestureRecognizerDelegate? https://codereview.chromium.org/2627093003/diff/40001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_context_menu_controller.mm (right): https://codereview.chromium.org/2627093003/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_context_menu_controller.mm:6: #include <stddef.h> This group of #imports/#includes should be after the #import of the counterpart header. https://codereview.chromium.org/2627093003/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_context_menu_controller.mm:36: } } // namespace
https://codereview.chromium.org/2627093003/diff/40001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/context_menu/context_menu_delegate.h (right): https://codereview.chromium.org/2627093003/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_delegate.h:16: - (BOOL)webState:(web::WebState*)webState First argument in Objective-C delegate method is always a caller, which is not really the case here. Should this delegate be like this?: @protocol OfflinePageNativeContentDelegate - (BOOL)dfflinePageNativeContent:(OfflinePageNativeContent*)nativeContent handleContextMenu:(const web::ContextMenuParams&)params; @end I know that Chrome for iOS code often reuses same delegate protocol for different classes, but that approach have a few issues: - it does not follow Coding Guidelines for Cocoa (which are basis for Google Style Guide) - it increases complexity, because it is harder to reason about the caller https://codereview.chromium.org/2627093003/diff/40001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/offline_page_native_content.h (right): https://codereview.chromium.org/2627093003/diff/40001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/offline_page_native_content.h:35: contextMenuDelegate:(id<ContextMenuDelegate>)contextMenuDelegate Would it make sense to change this to property instead of adding 5th argument? https://codereview.chromium.org/2627093003/diff/40001/ios/web/public/web_stat... File ios/web/public/web_state/ui/crw_context_menu_controller.h (right): https://codereview.chromium.org/2627093003/diff/40001/ios/web/public/web_stat... ios/web/public/web_state/ui/crw_context_menu_controller.h:18: @protocol CRWContextMenuControllerWebView Is there a reason why you want this to be flexible? CRWContextMenuController could work with UIWebView, but I don't think we would need that. Do you want to make CRWContextMenuController directly dependent on WKWebView to keep things simple? https://codereview.chromium.org/2627093003/diff/40001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_context_menu_controller.mm (right): https://codereview.chromium.org/2627093003/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_context_menu_controller.mm:18: // The long press detection duration must be shorter than the UIWebView's s/UIWebView/WKWebView https://codereview.chromium.org/2627093003/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_context_menu_controller.mm:92: // On iOS 4.x, there are two gesture recognizers on the UIWebView Could you please update this comment. https://codereview.chromium.org/2627093003/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_context_menu_controller.mm:141: if (!self.webScrollView) Can this be nil? https://codereview.chromium.org/2627093003/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_context_menu_controller.mm:147: // We don't want ongoing notification that the long press is held. nit: Could you please remove "we" https://codereview.chromium.org/2627093003/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_context_menu_controller.mm:165: // Set the specified recognizer to take priority over any recognizers in the If this method is needed (I personally think it's not), then can you predeclare it and move comments there? Also s/Set/Sets https://codereview.chromium.org/2627093003/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_context_menu_controller.mm:174: [iRecognizer requireGestureRecognizerToFail:recognizer]; Do we even get here on iOS9/10? https://codereview.chromium.org/2627093003/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_context_menu_controller.mm:285: // Do nothing for native views. Is this early return necessary? Comments seems to be related to CRWWebController behavior. https://codereview.chromium.org/2627093003/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_context_menu_controller.mm:301: if (!_webView) { Can this be nil?
Patchset #4 (id:60001) has been deleted
Thanks. PTAL https://codereview.chromium.org/2627093003/diff/40001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/browser_view_controller.mm (right): https://codereview.chromium.org/2627093003/diff/40001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/browser_view_controller.mm:297: ContextMenuDelegate, On 2017/01/12 17:13:12, jif wrote: > Does it make sense to use the CRWWebStateDelegate instead of > ContextMenuDelegate? I don't think so. CRWWebStateDelegate has other methods and this protocol is not intended to be delegate of a webstate https://codereview.chromium.org/2627093003/diff/40001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/context_menu/context_menu_delegate.h (right): https://codereview.chromium.org/2627093003/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/context_menu/context_menu_delegate.h:16: - (BOOL)webState:(web::WebState*)webState On 2017/01/13 07:16:26, Eugene But wrote: > First argument in Objective-C delegate method is always a caller, which is not > really the case here. Should this delegate be like this?: > > @protocol OfflinePageNativeContentDelegate > - (BOOL)dfflinePageNativeContent:(OfflinePageNativeContent*)nativeContent > handleContextMenu:(const web::ContextMenuParams&)params; > @end > > I know that Chrome for iOS code often reuses same delegate protocol for > different classes, but that approach have a few issues: > - it does not follow Coding Guidelines for Cocoa (which are basis for Google > Style Guide) > - it increases complexity, because it is harder to reason about the caller Done. I used the mode generic "native content" instead of OfflinePageNativeContent as I don't see any reason to limit to OfflinePageNativeContentDelegate. There is already a nativeContentDelegate, so I added my method there. I can change that if needed. https://codereview.chromium.org/2627093003/diff/40001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/offline_page_native_content.h (right): https://codereview.chromium.org/2627093003/diff/40001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/offline_page_native_content.h:35: contextMenuDelegate:(id<ContextMenuDelegate>)contextMenuDelegate On 2017/01/13 07:16:26, Eugene But wrote: > Would it make sense to change this to property instead of adding 5th argument? Removed it. Added to the existing delegate. https://codereview.chromium.org/2627093003/diff/40001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/offline_page_native_content.mm (right): https://codereview.chromium.org/2627093003/diff/40001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/offline_page_native_content.mm:108: On 2017/01/12 17:13:12, jif wrote: > #pragma mark - CRWContextMenuControllerDelegate ? Done (in staticHTMLViewController) https://codereview.chromium.org/2627093003/diff/40001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/static_content/static_html_view_controller.h (right): https://codereview.chromium.org/2627093003/diff/40001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/static_content/static_html_view_controller.h:76: // loading resources and must not be null. On 2017/01/12 17:13:12, jif wrote: > Say that |contextMenuDelegate| can be nil. Argument removed. https://codereview.chromium.org/2627093003/diff/40001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/static_content/static_html_view_controller.mm (right): https://codereview.chromium.org/2627093003/diff/40001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/static_content/static_html_view_controller.mm:49: UIGestureRecognizerDelegate> { On 2017/01/12 17:13:12, jif wrote: > remove UIGestureRecognizerDelegate? Done. https://codereview.chromium.org/2627093003/diff/40001/ios/web/public/web_stat... File ios/web/public/web_state/ui/crw_context_menu_controller.h (right): https://codereview.chromium.org/2627093003/diff/40001/ios/web/public/web_stat... ios/web/public/web_state/ui/crw_context_menu_controller.h:18: @protocol CRWContextMenuControllerWebView On 2017/01/13 07:16:27, Eugene But wrote: > Is there a reason why you want this to be flexible? CRWContextMenuController > could work with UIWebView, but I don't think we would need that. Do you want to > make CRWContextMenuController directly dependent on WKWebView to keep things > simple? I made this to depend directly on WKWebWebView. I let the javascript delegate as webcontroller adds some checks on it. I am not sure it is needed as we only call simple commands here, but I did not want to drop the check silently. https://codereview.chromium.org/2627093003/diff/40001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_context_menu_controller.mm (right): https://codereview.chromium.org/2627093003/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_context_menu_controller.mm:6: #include <stddef.h> On 2017/01/12 17:13:12, jif wrote: > This group of #imports/#includes should be after the #import of the counterpart > header. Done. https://codereview.chromium.org/2627093003/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_context_menu_controller.mm:18: // The long press detection duration must be shorter than the UIWebView's On 2017/01/13 07:16:27, Eugene But wrote: > s/UIWebView/WKWebView Done. https://codereview.chromium.org/2627093003/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_context_menu_controller.mm:36: } On 2017/01/12 17:13:12, jif wrote: > } // namespace Done. https://codereview.chromium.org/2627093003/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_context_menu_controller.mm:92: // On iOS 4.x, there are two gesture recognizers on the UIWebView On 2017/01/13 07:16:27, Eugene But wrote: > Could you please update this comment. Done. https://codereview.chromium.org/2627093003/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_context_menu_controller.mm:141: if (!self.webScrollView) On 2017/01/13 07:16:27, Eugene But wrote: > Can this be nil? I don't think so but documentation on this property is really small. https://codereview.chromium.org/2627093003/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_context_menu_controller.mm:147: // We don't want ongoing notification that the long press is held. On 2017/01/13 07:16:27, Eugene But wrote: > nit: Could you please remove "we" Done. https://codereview.chromium.org/2627093003/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_context_menu_controller.mm:165: // Set the specified recognizer to take priority over any recognizers in the On 2017/01/13 07:16:27, Eugene But wrote: > If this method is needed (I personally think it's not), then can you predeclare > it and move comments there? Also s/Set/Sets I checked. There seems to be no gesture recognizers handled by this method anymore. However, I would prefer to have this CL making no change for web controller. If we find out that this call was needed, the revert will be easier. I can remove this call in a next CL https://codereview.chromium.org/2627093003/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_context_menu_controller.mm:165: // Set the specified recognizer to take priority over any recognizers in the On 2017/01/13 07:16:27, Eugene But wrote: > If this method is needed (I personally think it's not), then can you predeclare > it and move comments there? Also s/Set/Sets Done. https://codereview.chromium.org/2627093003/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_context_menu_controller.mm:174: [iRecognizer requireGestureRecognizerToFail:recognizer]; On 2017/01/13 07:16:27, Eugene But wrote: > Do we even get here on iOS9/10? Tested and apparently no (iphone iOS9/10). https://codereview.chromium.org/2627093003/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_context_menu_controller.mm:285: // Do nothing for native views. On 2017/01/13 07:16:27, Eugene But wrote: > Is this early return necessary? Comments seems to be related to CRWWebController > behavior. No, removed. https://codereview.chromium.org/2627093003/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_context_menu_controller.mm:301: if (!_webView) { On 2017/01/13 07:16:27, Eugene But wrote: > Can this be nil? Done. https://codereview.chromium.org/2627093003/diff/80001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/static_content/static_html_view_controller.mm (right): https://codereview.chromium.org/2627093003/diff/80001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/static_content/static_html_view_controller.mm:220: return [delegate_ nativeContent:self handleContextMenu:params]; I kept the current behavior of the delegate to pass the StaticHtmlViewController instead of the nativeContent. The nativeContent is not used anyway, but if you think it is better to forward the call to the actual nativeContent, I can do it.
Generally this LG, have a couple of implementation comments. Also could you please write some tests in a follow up CL. https://codereview.chromium.org/2627093003/diff/80001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/static_content/static_html_view_controller.mm (right): https://codereview.chromium.org/2627093003/diff/80001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/static_content/static_html_view_controller.mm:215: #pragma mark CRWContextMenuDelegate implementation nit: Add a linebreak ? https://codereview.chromium.org/2627093003/diff/80001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/static_content/static_html_view_controller.mm:220: return [delegate_ nativeContent:self handleContextMenu:params]; On 2017/01/13 10:20:26, Olivier Robin wrote: > I kept the current behavior of the delegate to pass the StaticHtmlViewController > instead of the nativeContent. > The nativeContent is not used anyway, but if you think it is better to forward > the call to the actual nativeContent, I can do it. Looks consistent with -[CRWWebController nativeContent:titleDidChange:] and cleaner than my original suggestion, so your change lg :) https://codereview.chromium.org/2627093003/diff/80001/ios/web/public/web_stat... File ios/web/public/web_state/ui/crw_context_menu_delegate.h (right): https://codereview.chromium.org/2627093003/diff/80001/ios/web/public/web_stat... ios/web/public/web_state/ui/crw_context_menu_delegate.h:13: - (BOOL)webView:(WKWebView*)webView Could you please add comments. Things which worth explaining are: - this is called when user performs a long press gesture on a web view - this method is not called if gesture is performed on iframe (crbug.com/228179) - if result is YES then system context menu will be suppressed - if result is NO then system context menu will show up - clients don't have to show any UI, so this can be used to suppress system context menu Maybe other things I missed here? https://codereview.chromium.org/2627093003/diff/80001/ios/web/public/web_stat... File ios/web/public/web_state/ui/crw_native_content.h (right): https://codereview.chromium.org/2627093003/diff/80001/ios/web/public/web_stat... ios/web/public/web_state/ui/crw_native_content.h:109: // Return if the context menu was handled. Did you mean "Returns NO if the context menu was handled." ? https://codereview.chromium.org/2627093003/diff/80001/ios/web/public/web_view... File ios/web/public/web_view_creation_util.h (right): https://codereview.chromium.org/2627093003/diff/80001/ios/web/public/web_view... ios/web/public/web_view_creation_util.h:32: // nil is equivalent to |BuildWKWebView|. Attaching content menu recognized has an actual performance implications (JS will be executed on literally every touch). Please document that, so clients are aware about performance tradeoff. https://codereview.chromium.org/2627093003/diff/80001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_context_menu_controller.h (right): https://codereview.chromium.org/2627093003/diff/80001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_context_menu_controller.h:8: #import <UIKit/UIKit.h> nit: there is nothing from UIKit in this header, importing Foundation is enough https://codereview.chromium.org/2627093003/diff/80001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_context_menu_controller.h:20: @interface CRWContextMenuController : NSObject Per Style Guide please add comments to this class and public method. Things worth mentioning: - webView is not retained and can not be nil - javaScriptDelegate can be nil - what happens if javaScriptDelegate is nil - why clients would want to pass non-nil javaScriptDelegate - this class installs gesture recognizers - this class has performance implications (JS is executed on every view touch) - this class invokes CRWContextMenuDelegate methods only if long press happened on the main frame (crbug.com/228179) Maybe other things I missed here? https://codereview.chromium.org/2627093003/diff/80001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_context_menu_controller.h:24: (id<CRWContextMenuJavaScriptDelegate>)javaScriptDelegate You can use CRWJSInjectionEvaluator protocol here, there are not many advantages in a separate delegate as this code is very isolated. https://codereview.chromium.org/2627093003/diff/80001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_context_menu_controller.mm (right): https://codereview.chromium.org/2627093003/diff/80001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_context_menu_controller.mm:6: Can this file be ARC? I't totally reasonable to make the change in a separate CL. https://codereview.chromium.org/2627093003/diff/80001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_context_menu_controller.mm:49: @property(nonatomic, readonly) UIScrollView* webScrollView; nit: do you need this property? I would drop it, but it's up to you. https://codereview.chromium.org/2627093003/diff/80001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_context_menu_controller.mm:93: // @"title", @"referrerPolicy". All values are strings. Used for showing I would drop "Used for showing context menus.". That was useful in the context of CRWWebController, now it's quite obvious. https://codereview.chromium.org/2627093003/diff/80001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_context_menu_controller.mm:106: self = [super init]; DCHECK(webView) https://codereview.chromium.org/2627093003/diff/80001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_context_menu_controller.mm:135: // TODO(crbug.com/680930): This code is not needed anymore in iOS9+ and will super nil: Drop "will" https://codereview.chromium.org/2627093003/diff/80001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2627093003/diff/80001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:5118: return self.webStateImpl->HandleContextMenu(params); This can be called asynchronously, when WebState is already destroyed, please check _isBeingDestroyed befor calling HandleContextMenu. https://codereview.chromium.org/2627093003/diff/80001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:5122: #pragma mark CRWNativeContentDelegate methods nit: Add a linebreak https://codereview.chromium.org/2627093003/diff/80001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:5125: return self.webStateImpl->HandleContextMenu(params); Please check _isBeingDestroyed https://codereview.chromium.org/2627093003/diff/80001/ios/web/web_state/web_v... File ios/web/web_state/web_view_internal_creation_util.mm (right): https://codereview.chromium.org/2627093003/diff/80001/ios/web/web_state/web_v... ios/web/web_state/web_view_internal_creation_util.mm:5: #import <objc/runtime.h> This import should go after /web_view_internal_creation_util.h
Thanks ! PTAL https://codereview.chromium.org/2627093003/diff/80001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/static_content/static_html_view_controller.mm (right): https://codereview.chromium.org/2627093003/diff/80001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/static_content/static_html_view_controller.mm:215: #pragma mark CRWContextMenuDelegate implementation On 2017/01/13 16:19:41, Eugene But wrote: > nit: Add a linebreak ? Done. https://codereview.chromium.org/2627093003/diff/80001/ios/web/public/web_stat... File ios/web/public/web_state/ui/crw_context_menu_delegate.h (right): https://codereview.chromium.org/2627093003/diff/80001/ios/web/public/web_stat... ios/web/public/web_state/ui/crw_context_menu_delegate.h:13: - (BOOL)webView:(WKWebView*)webView On 2017/01/13 16:19:41, Eugene But wrote: > Could you please add comments. Things which worth explaining are: > - this is called when user performs a long press gesture on a web view > - this method is not called if gesture is performed on iframe (crbug.com/228179) > - if result is YES then system context menu will be suppressed > - if result is NO then system context menu will show up > - clients don't have to show any UI, so this can be used to suppress system > context menu > > Maybe other things I missed here? Added a timing comment when returning YES. https://codereview.chromium.org/2627093003/diff/80001/ios/web/public/web_stat... File ios/web/public/web_state/ui/crw_native_content.h (right): https://codereview.chromium.org/2627093003/diff/80001/ios/web/public/web_stat... ios/web/public/web_state/ui/crw_native_content.h:109: // Return if the context menu was handled. On 2017/01/13 16:19:41, Eugene But wrote: > Did you mean "Returns NO if the context menu was handled." ? Done. https://codereview.chromium.org/2627093003/diff/80001/ios/web/public/web_view... File ios/web/public/web_view_creation_util.h (right): https://codereview.chromium.org/2627093003/diff/80001/ios/web/public/web_view... ios/web/public/web_view_creation_util.h:32: // nil is equivalent to |BuildWKWebView|. On 2017/01/13 16:19:41, Eugene But wrote: > Attaching content menu recognized has an actual performance implications (JS > will be executed on literally every touch). Please document that, so clients are > aware about performance tradeoff. Done. https://codereview.chromium.org/2627093003/diff/80001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_context_menu_controller.h (right): https://codereview.chromium.org/2627093003/diff/80001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_context_menu_controller.h:8: #import <UIKit/UIKit.h> On 2017/01/13 16:19:41, Eugene But wrote: > nit: there is nothing from UIKit in this header, importing Foundation is enough I thought WKWebView needed it. Done. https://codereview.chromium.org/2627093003/diff/80001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_context_menu_controller.h:20: @interface CRWContextMenuController : NSObject On 2017/01/13 16:19:41, Eugene But wrote: > Per Style Guide please add comments to this class and public method. Things > worth mentioning: > - webView is not retained and can not be nil > - javaScriptDelegate can be nil > - what happens if javaScriptDelegate is nil > - why clients would want to pass non-nil javaScriptDelegate > - this class installs gesture recognizers > - this class has performance implications (JS is executed on every view touch) > - this class invokes CRWContextMenuDelegate methods only if long press happened > on the main frame (crbug.com/228179) > > Maybe other things I missed here? Done. https://codereview.chromium.org/2627093003/diff/80001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_context_menu_controller.h:24: (id<CRWContextMenuJavaScriptDelegate>)javaScriptDelegate On 2017/01/13 16:19:41, Eugene But wrote: > You can use CRWJSInjectionEvaluator protocol here, there are not many advantages > in a separate delegate as this code is very isolated. Done. https://codereview.chromium.org/2627093003/diff/80001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_context_menu_controller.mm (right): https://codereview.chromium.org/2627093003/diff/80001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_context_menu_controller.mm:6: On 2017/01/13 16:19:42, Eugene But wrote: > Can this file be ARC? I't totally reasonable to make the change in a separate > CL. I tried and there was a problem with objc_setAssociatedObject. I will ask stk in another CL. https://codereview.chromium.org/2627093003/diff/80001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_context_menu_controller.mm:49: @property(nonatomic, readonly) UIScrollView* webScrollView; On 2017/01/13 16:19:42, Eugene But wrote: > nit: do you need this property? I would drop it, but it's up to you. Not really. https://codereview.chromium.org/2627093003/diff/80001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_context_menu_controller.mm:93: // @"title", @"referrerPolicy". All values are strings. Used for showing On 2017/01/13 16:19:42, Eugene But wrote: > I would drop "Used for showing context menus.". That was useful in the context > of CRWWebController, now it's quite obvious. Done. https://codereview.chromium.org/2627093003/diff/80001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_context_menu_controller.mm:106: self = [super init]; On 2017/01/13 16:19:42, Eugene But wrote: > DCHECK(webView) Done. https://codereview.chromium.org/2627093003/diff/80001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_context_menu_controller.mm:135: // TODO(crbug.com/680930): This code is not needed anymore in iOS9+ and will On 2017/01/13 16:19:42, Eugene But wrote: > super nil: Drop "will" Done. https://codereview.chromium.org/2627093003/diff/80001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2627093003/diff/80001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:5118: return self.webStateImpl->HandleContextMenu(params); On 2017/01/13 16:19:42, Eugene But wrote: > This can be called asynchronously, when WebState is already destroyed, please > check _isBeingDestroyed befor calling HandleContextMenu. Done. https://codereview.chromium.org/2627093003/diff/80001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:5122: #pragma mark CRWNativeContentDelegate methods On 2017/01/13 16:19:42, Eugene But wrote: > nit: Add a linebreak Done. https://codereview.chromium.org/2627093003/diff/80001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:5125: return self.webStateImpl->HandleContextMenu(params); On 2017/01/13 16:19:42, Eugene But wrote: > Please check _isBeingDestroyed Done. https://codereview.chromium.org/2627093003/diff/80001/ios/web/web_state/web_v... File ios/web/web_state/web_view_internal_creation_util.mm (right): https://codereview.chromium.org/2627093003/diff/80001/ios/web/web_state/web_v... ios/web/web_state/web_view_internal_creation_util.mm:5: #import <objc/runtime.h> On 2017/01/13 16:19:42, Eugene But wrote: > This import should go after /web_view_internal_creation_util.h Done.
On 2017/01/13 18:08:59, Olivier Robin wrote: > Thanks ! > PTAL > > https://codereview.chromium.org/2627093003/diff/80001/ios/chrome/browser/ui/s... > File ios/chrome/browser/ui/static_content/static_html_view_controller.mm > (right): > > https://codereview.chromium.org/2627093003/diff/80001/ios/chrome/browser/ui/s... > ios/chrome/browser/ui/static_content/static_html_view_controller.mm:215: #pragma > mark CRWContextMenuDelegate implementation > On 2017/01/13 16:19:41, Eugene But wrote: > > nit: Add a linebreak ? > > Done. > > https://codereview.chromium.org/2627093003/diff/80001/ios/web/public/web_stat... > File ios/web/public/web_state/ui/crw_context_menu_delegate.h (right): > > https://codereview.chromium.org/2627093003/diff/80001/ios/web/public/web_stat... > ios/web/public/web_state/ui/crw_context_menu_delegate.h:13: - > (BOOL)webView:(WKWebView*)webView > On 2017/01/13 16:19:41, Eugene But wrote: > > Could you please add comments. Things which worth explaining are: > > - this is called when user performs a long press gesture on a web view > > - this method is not called if gesture is performed on iframe > (crbug.com/228179) > > - if result is YES then system context menu will be suppressed > > - if result is NO then system context menu will show up > > - clients don't have to show any UI, so this can be used to suppress system > > context menu > > > > Maybe other things I missed here? > > Added a timing comment when returning YES. > > https://codereview.chromium.org/2627093003/diff/80001/ios/web/public/web_stat... > File ios/web/public/web_state/ui/crw_native_content.h (right): > > https://codereview.chromium.org/2627093003/diff/80001/ios/web/public/web_stat... > ios/web/public/web_state/ui/crw_native_content.h:109: // Return if the context > menu was handled. > On 2017/01/13 16:19:41, Eugene But wrote: > > Did you mean "Returns NO if the context menu was handled." ? > > Done. > > https://codereview.chromium.org/2627093003/diff/80001/ios/web/public/web_view... > File ios/web/public/web_view_creation_util.h (right): > > https://codereview.chromium.org/2627093003/diff/80001/ios/web/public/web_view... > ios/web/public/web_view_creation_util.h:32: // nil is equivalent to > |BuildWKWebView|. > On 2017/01/13 16:19:41, Eugene But wrote: > > Attaching content menu recognized has an actual performance implications (JS > > will be executed on literally every touch). Please document that, so clients > are > > aware about performance tradeoff. > > Done. > > https://codereview.chromium.org/2627093003/diff/80001/ios/web/web_state/ui/cr... > File ios/web/web_state/ui/crw_context_menu_controller.h (right): > > https://codereview.chromium.org/2627093003/diff/80001/ios/web/web_state/ui/cr... > ios/web/web_state/ui/crw_context_menu_controller.h:8: #import <UIKit/UIKit.h> > On 2017/01/13 16:19:41, Eugene But wrote: > > nit: there is nothing from UIKit in this header, importing Foundation is > enough > > I thought WKWebView needed it. > Done. > > https://codereview.chromium.org/2627093003/diff/80001/ios/web/web_state/ui/cr... > ios/web/web_state/ui/crw_context_menu_controller.h:20: @interface > CRWContextMenuController : NSObject > On 2017/01/13 16:19:41, Eugene But wrote: > > Per Style Guide please add comments to this class and public method. Things > > worth mentioning: > > - webView is not retained and can not be nil > > - javaScriptDelegate can be nil > > - what happens if javaScriptDelegate is nil > > - why clients would want to pass non-nil javaScriptDelegate > > - this class installs gesture recognizers > > - this class has performance implications (JS is executed on every view > touch) > > - this class invokes CRWContextMenuDelegate methods only if long press > happened > > on the main frame (crbug.com/228179) > > > > Maybe other things I missed here? > > Done. > > https://codereview.chromium.org/2627093003/diff/80001/ios/web/web_state/ui/cr... > ios/web/web_state/ui/crw_context_menu_controller.h:24: > (id<CRWContextMenuJavaScriptDelegate>)javaScriptDelegate > On 2017/01/13 16:19:41, Eugene But wrote: > > You can use CRWJSInjectionEvaluator protocol here, there are not many > advantages > > in a separate delegate as this code is very isolated. > > Done. > > https://codereview.chromium.org/2627093003/diff/80001/ios/web/web_state/ui/cr... > File ios/web/web_state/ui/crw_context_menu_controller.mm (right): > > https://codereview.chromium.org/2627093003/diff/80001/ios/web/web_state/ui/cr... > ios/web/web_state/ui/crw_context_menu_controller.mm:6: > On 2017/01/13 16:19:42, Eugene But wrote: > > Can this file be ARC? I't totally reasonable to make the change in a separate > > CL. > > I tried and there was a problem with objc_setAssociatedObject. > I will ask stk in another CL. > > https://codereview.chromium.org/2627093003/diff/80001/ios/web/web_state/ui/cr... > ios/web/web_state/ui/crw_context_menu_controller.mm:49: @property(nonatomic, > readonly) UIScrollView* webScrollView; > On 2017/01/13 16:19:42, Eugene But wrote: > > nit: do you need this property? I would drop it, but it's up to you. > > Not really. > > https://codereview.chromium.org/2627093003/diff/80001/ios/web/web_state/ui/cr... > ios/web/web_state/ui/crw_context_menu_controller.mm:93: // @"title", > @"referrerPolicy". All values are strings. Used for showing > On 2017/01/13 16:19:42, Eugene But wrote: > > I would drop "Used for showing context menus.". That was useful in the context > > of CRWWebController, now it's quite obvious. > > Done. > > https://codereview.chromium.org/2627093003/diff/80001/ios/web/web_state/ui/cr... > ios/web/web_state/ui/crw_context_menu_controller.mm:106: self = [super init]; > On 2017/01/13 16:19:42, Eugene But wrote: > > DCHECK(webView) > > Done. > > https://codereview.chromium.org/2627093003/diff/80001/ios/web/web_state/ui/cr... > ios/web/web_state/ui/crw_context_menu_controller.mm:135: // > TODO(crbug.com/680930): This code is not needed anymore in iOS9+ and will > On 2017/01/13 16:19:42, Eugene But wrote: > > super nil: Drop "will" > > Done. > > https://codereview.chromium.org/2627093003/diff/80001/ios/web/web_state/ui/cr... > File ios/web/web_state/ui/crw_web_controller.mm (right): > > https://codereview.chromium.org/2627093003/diff/80001/ios/web/web_state/ui/cr... > ios/web/web_state/ui/crw_web_controller.mm:5118: return > self.webStateImpl->HandleContextMenu(params); > On 2017/01/13 16:19:42, Eugene But wrote: > > This can be called asynchronously, when WebState is already destroyed, please > > check _isBeingDestroyed befor calling HandleContextMenu. > > Done. > > https://codereview.chromium.org/2627093003/diff/80001/ios/web/web_state/ui/cr... > ios/web/web_state/ui/crw_web_controller.mm:5122: #pragma mark > CRWNativeContentDelegate methods > On 2017/01/13 16:19:42, Eugene But wrote: > > nit: Add a linebreak > > Done. > > https://codereview.chromium.org/2627093003/diff/80001/ios/web/web_state/ui/cr... > ios/web/web_state/ui/crw_web_controller.mm:5125: return > self.webStateImpl->HandleContextMenu(params); > On 2017/01/13 16:19:42, Eugene But wrote: > > Please check _isBeingDestroyed > > Done. > > https://codereview.chromium.org/2627093003/diff/80001/ios/web/web_state/web_v... > File ios/web/web_state/web_view_internal_creation_util.mm (right): > > https://codereview.chromium.org/2627093003/diff/80001/ios/web/web_state/web_v... > ios/web/web_state/web_view_internal_creation_util.mm:5: #import <objc/runtime.h> > On 2017/01/13 16:19:42, Eugene But wrote: > > This import should go after /web_view_internal_creation_util.h > > Done. Sorry, I was too fast to send message. It is not ready.
https://codereview.chromium.org/2627093003/diff/100001/ios/web/public/web_sta... File ios/web/public/web_state/ui/crw_context_menu_delegate.h (right): https://codereview.chromium.org/2627093003/diff/100001/ios/web/public/web_sta... ios/web/public/web_state/ui/crw_context_menu_delegate.h:19: // YES must be returned within 0.05 second. Is it a requirement to return YES in 0.05 seconds? I thought that for synchronous call it does not matter. https://codereview.chromium.org/2627093003/diff/100001/ios/web/public/web_sta... ios/web/public/web_state/ui/crw_context_menu_delegate.h:20: // - if result is NO, the system context meny will be displayed. s/meny/menu https://codereview.chromium.org/2627093003/diff/100001/ios/web/public/web_sta... ios/web/public/web_state/ui/crw_context_menu_delegate.h:22: // menu. Please also document that method is not called if gesture is performed on iframe (crbug.com/228179) https://codereview.chromium.org/2627093003/diff/100001/ios/web/public/web_sta... File ios/web/public/web_state/ui/crw_native_content.h (right): https://codereview.chromium.org/2627093003/diff/100001/ios/web/public/web_sta... ios/web/public/web_state/ui/crw_native_content.h:110: // system menu must be suppressed. Please document the meaning of BOOL result. https://codereview.chromium.org/2627093003/diff/100001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_context_menu_controller.h (right): https://codereview.chromium.org/2627093003/diff/100001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_context_menu_controller.h:22: // Install the |CRWContextMenuController| on |webView|. s/Install/Installs https://codereview.chromium.org/2627093003/diff/100001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_context_menu_controller.h:23: // - |webView| cannot be nil. Please document that |webView| is not retained and callers are responsible for keeping it alive. https://codereview.chromium.org/2627093003/diff/100001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_context_menu_controller.h:26: // is directly called on |webView|. nit: s/called/executed https://codereview.chromium.org/2627093003/diff/100001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_context_menu_controller.h:32: javaScriptDelegate: s/javaScriptDelegate/injectionEvaluator https://codereview.chromium.org/2627093003/diff/100001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_context_menu_controller.h:33: (id<CRWJSInjectionEvaluator>)javaScriptDelegate ditto https://codereview.chromium.org/2627093003/diff/100001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_context_menu_controller.mm (right): https://codereview.chromium.org/2627093003/diff/100001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_context_menu_controller.mm:84: WKWebView* _webView; Sorry, missed in the previous round. If this can not be ARC file, then please use WeakNSObject for all weak members. This class works asynchronously and web view and delegates may die at any moment. https://codereview.chromium.org/2627093003/diff/100001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_context_menu_controller.mm:102: (id<CRWContextMenuJavaScriptDelegate>)javaScriptDelegate Evaluator? https://codereview.chromium.org/2627093003/diff/100001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2627093003/diff/100001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_web_controller.mm:281: CRWContextMenuJavaScriptDelegate, Do you need this?
On 2017/01/13 18:23:42, Eugene But wrote: > https://codereview.chromium.org/2627093003/diff/100001/ios/web/public/web_sta... > File ios/web/public/web_state/ui/crw_context_menu_delegate.h (right): > > https://codereview.chromium.org/2627093003/diff/100001/ios/web/public/web_sta... > ios/web/public/web_state/ui/crw_context_menu_delegate.h:19: // YES must be > returned within 0.05 second. > Is it a requirement to return YES in 0.05 seconds? I thought that for > synchronous call it does not matter. > > https://codereview.chromium.org/2627093003/diff/100001/ios/web/public/web_sta... > ios/web/public/web_state/ui/crw_context_menu_delegate.h:20: // - if result is > NO, the system context meny will be displayed. > s/meny/menu > > https://codereview.chromium.org/2627093003/diff/100001/ios/web/public/web_sta... > ios/web/public/web_state/ui/crw_context_menu_delegate.h:22: // menu. > Please also document that method is not called if gesture is performed on iframe > (crbug.com/228179) > > https://codereview.chromium.org/2627093003/diff/100001/ios/web/public/web_sta... > File ios/web/public/web_state/ui/crw_native_content.h (right): > > https://codereview.chromium.org/2627093003/diff/100001/ios/web/public/web_sta... > ios/web/public/web_state/ui/crw_native_content.h:110: // system menu must be > suppressed. > Please document the meaning of BOOL result. > > https://codereview.chromium.org/2627093003/diff/100001/ios/web/web_state/ui/c... > File ios/web/web_state/ui/crw_context_menu_controller.h (right): > > https://codereview.chromium.org/2627093003/diff/100001/ios/web/web_state/ui/c... > ios/web/web_state/ui/crw_context_menu_controller.h:22: // Install the > |CRWContextMenuController| on |webView|. > s/Install/Installs > > https://codereview.chromium.org/2627093003/diff/100001/ios/web/web_state/ui/c... > ios/web/web_state/ui/crw_context_menu_controller.h:23: // - |webView| cannot be > nil. > Please document that |webView| is not retained and callers are responsible for > keeping it alive. > > https://codereview.chromium.org/2627093003/diff/100001/ios/web/web_state/ui/c... > ios/web/web_state/ui/crw_context_menu_controller.h:26: // is directly called > on |webView|. > nit: s/called/executed > > https://codereview.chromium.org/2627093003/diff/100001/ios/web/web_state/ui/c... > ios/web/web_state/ui/crw_context_menu_controller.h:32: javaScriptDelegate: > s/javaScriptDelegate/injectionEvaluator > > https://codereview.chromium.org/2627093003/diff/100001/ios/web/web_state/ui/c... > ios/web/web_state/ui/crw_context_menu_controller.h:33: > (id<CRWJSInjectionEvaluator>)javaScriptDelegate > ditto > > https://codereview.chromium.org/2627093003/diff/100001/ios/web/web_state/ui/c... > File ios/web/web_state/ui/crw_context_menu_controller.mm (right): > > https://codereview.chromium.org/2627093003/diff/100001/ios/web/web_state/ui/c... > ios/web/web_state/ui/crw_context_menu_controller.mm:84: WKWebView* _webView; > Sorry, missed in the previous round. If this can not be ARC file, then please > use WeakNSObject for all weak members. This class works asynchronously and web > view and delegates may die at any moment. > > https://codereview.chromium.org/2627093003/diff/100001/ios/web/web_state/ui/c... > ios/web/web_state/ui/crw_context_menu_controller.mm:102: > (id<CRWContextMenuJavaScriptDelegate>)javaScriptDelegate > Evaluator? > > https://codereview.chromium.org/2627093003/diff/100001/ios/web/web_state/ui/c... > File ios/web/web_state/ui/crw_web_controller.mm (right): > > https://codereview.chromium.org/2627093003/diff/100001/ios/web/web_state/ui/c... > ios/web/web_state/ui/crw_web_controller.mm:281: > CRWContextMenuJavaScriptDelegate, > Do you need this? Sorry, already reviewed :) But there was important thing I missed in the previous rounds (using WeakNSObject for webView and delegates).
Patchset #6 (id:120001) has been deleted
Thanks. I will add unittests on Monday in another CL. https://codereview.chromium.org/2627093003/diff/100001/ios/web/public/web_sta... File ios/web/public/web_state/ui/crw_context_menu_delegate.h (right): https://codereview.chromium.org/2627093003/diff/100001/ios/web/public/web_sta... ios/web/public/web_state/ui/crw_context_menu_delegate.h:19: // YES must be returned within 0.05 second. On 2017/01/13 18:23:42, Eugene But wrote: > Is it a requirement to return YES in 0.05 seconds? I thought that for > synchronous call it does not matter. At least if you spin the runloop, it will be a problem. https://codereview.chromium.org/2627093003/diff/100001/ios/web/public/web_sta... ios/web/public/web_state/ui/crw_context_menu_delegate.h:20: // - if result is NO, the system context meny will be displayed. On 2017/01/13 18:23:42, Eugene But wrote: > s/meny/menu Done. https://codereview.chromium.org/2627093003/diff/100001/ios/web/public/web_sta... ios/web/public/web_state/ui/crw_context_menu_delegate.h:22: // menu. On 2017/01/13 18:23:42, Eugene But wrote: > Please also document that method is not called if gesture is performed on iframe > (crbug.com/228179) Done. https://codereview.chromium.org/2627093003/diff/100001/ios/web/public/web_sta... File ios/web/public/web_state/ui/crw_native_content.h (right): https://codereview.chromium.org/2627093003/diff/100001/ios/web/public/web_sta... ios/web/public/web_state/ui/crw_native_content.h:110: // system menu must be suppressed. On 2017/01/13 18:23:42, Eugene But wrote: > Please document the meaning of BOOL result. Is the last sentence not enough? https://codereview.chromium.org/2627093003/diff/100001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_context_menu_controller.h (right): https://codereview.chromium.org/2627093003/diff/100001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_context_menu_controller.h:22: // Install the |CRWContextMenuController| on |webView|. On 2017/01/13 18:23:42, Eugene But wrote: > s/Install/Installs Done. https://codereview.chromium.org/2627093003/diff/100001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_context_menu_controller.h:23: // - |webView| cannot be nil. On 2017/01/13 18:23:42, Eugene But wrote: > Please document that |webView| is not retained and callers are responsible for > keeping it alive. Done. https://codereview.chromium.org/2627093003/diff/100001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_context_menu_controller.h:26: // is directly called on |webView|. On 2017/01/13 18:23:42, Eugene But wrote: > nit: s/called/executed Done. https://codereview.chromium.org/2627093003/diff/100001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_context_menu_controller.h:32: javaScriptDelegate: On 2017/01/13 18:23:42, Eugene But wrote: > s/javaScriptDelegate/injectionEvaluator Done. https://codereview.chromium.org/2627093003/diff/100001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_context_menu_controller.h:33: (id<CRWJSInjectionEvaluator>)javaScriptDelegate On 2017/01/13 18:23:42, Eugene But wrote: > ditto Done. https://codereview.chromium.org/2627093003/diff/100001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_context_menu_controller.mm (right): https://codereview.chromium.org/2627093003/diff/100001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_context_menu_controller.mm:84: WKWebView* _webView; On 2017/01/13 18:23:42, Eugene But wrote: > Sorry, missed in the previous round. If this can not be ARC file, then please > use WeakNSObject for all weak members. This class works asynchronously and web > view and delegates may die at any moment. Done. https://codereview.chromium.org/2627093003/diff/100001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_context_menu_controller.mm:102: (id<CRWContextMenuJavaScriptDelegate>)javaScriptDelegate On 2017/01/13 18:23:42, Eugene But wrote: > Evaluator? Done. https://codereview.chromium.org/2627093003/diff/100001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2627093003/diff/100001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_web_controller.mm:281: CRWContextMenuJavaScriptDelegate, On 2017/01/13 18:23:42, Eugene But wrote: > Do you need this? Done.
lgtm Thank you for making WebController smaller. https://codereview.chromium.org/2627093003/diff/100001/ios/web/public/web_sta... File ios/web/public/web_state/ui/crw_context_menu_delegate.h (right): https://codereview.chromium.org/2627093003/diff/100001/ios/web/public/web_sta... ios/web/public/web_state/ui/crw_context_menu_delegate.h:19: // YES must be returned within 0.05 second. On 2017/01/13 18:49:55, Olivier Robin wrote: > On 2017/01/13 18:23:42, Eugene But wrote: > > Is it a requirement to return YES in 0.05 seconds? I thought that for > > synchronous call it does not matter. > > At least if you spin the runloop, it will be a problem. Thanks! So should this comment be "YES must be returned from the same runloop"? https://codereview.chromium.org/2627093003/diff/100001/ios/web/public/web_sta... File ios/web/public/web_state/ui/crw_native_content.h (right): https://codereview.chromium.org/2627093003/diff/100001/ios/web/public/web_sta... ios/web/public/web_state/ui/crw_native_content.h:110: // system menu must be suppressed. On 2017/01/13 18:49:55, Olivier Robin wrote: > On 2017/01/13 18:23:42, Eugene But wrote: > > Please document the meaning of BOOL result. > > Is the last sentence not enough? Sorry, it is enough. https://codereview.chromium.org/2627093003/diff/140001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2627093003/diff/140001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_web_controller.mm:281: CRWJSInjectionEvaluator, nit: You don't need this either: https://cs.chromium.org/chromium/src/ios/web/web_state/ui/crw_web_controller....
Done. Thanks! https://codereview.chromium.org/2627093003/diff/100001/ios/web/public/web_sta... File ios/web/public/web_state/ui/crw_context_menu_delegate.h (right): https://codereview.chromium.org/2627093003/diff/100001/ios/web/public/web_sta... ios/web/public/web_state/ui/crw_context_menu_delegate.h:19: // YES must be returned within 0.05 second. On 2017/01/13 19:00:32, Eugene But wrote: > On 2017/01/13 18:49:55, Olivier Robin wrote: > > On 2017/01/13 18:23:42, Eugene But wrote: > > > Is it a requirement to return YES in 0.05 seconds? I thought that for > > > synchronous call it does not matter. > > > > At least if you spin the runloop, it will be a problem. > Thanks! So should this comment be "YES must be returned from the same runloop"? Done. https://codereview.chromium.org/2627093003/diff/140001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2627093003/diff/140001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_web_controller.mm:281: CRWJSInjectionEvaluator, On 2017/01/13 19:00:32, Eugene But wrote: > nit: You don't need this either: > https://cs.chromium.org/chromium/src/ios/web/web_state/ui/crw_web_controller.... Done.
The CQ bit was checked by olivierrobin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eugenebut@chromium.org Link to the patchset: https://codereview.chromium.org/2627093003/#ps160001 (title: "feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1484555472134100,
"parent_rev": "716fc5a4985347e1b2148b481278d0d37d2bb3d3", "commit_rev":
"d9ca613db4a8616806d3447db5651870ed7c43fe"}
Message was sent while issue was closed.
Description was changed from ========== Reuse context menu in StaticHTMLViewController Create a CRWContextMenuController using the logic from CRWWebController. Reuse this class for StaticHTMLViewController. BUG=679818, 679793 ========== to ========== Reuse context menu in StaticHTMLViewController Create a CRWContextMenuController using the logic from CRWWebController. Reuse this class for StaticHTMLViewController. BUG=679818, 679793 Review-Url: https://codereview.chromium.org/2627093003 Cr-Commit-Position: refs/heads/master@{#443861} Committed: https://chromium.googlesource.com/chromium/src/+/d9ca613db4a8616806d3447db565... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:160001) as https://chromium.googlesource.com/chromium/src/+/d9ca613db4a8616806d3447db565... |
