Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2811073005:
Replace requirePageReconstruction with setCustomUserAgent (Closed)
|
Created:
3 years, 8 months ago by liaoyuke Modified:
3 years, 8 months ago CC:
chromium-reviews, ios-reviews+chrome_chromium.org, ios-reviews_chromium.org, pkl (ping after 24h if needed), ios-reviews+web_chromium.org, noyau+watch_chromium.org, marq+watch_chromium.org, sdefresne+watch_chromium.org, sdefresne Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionReplace requirePageReconstruction with setCustomUserAgent
WKWebView provides a property: customUserAgent that we can use to
change the user agent associated with the web view, which means that we
don't have to requirePageConstruction anymore when user agent changes.
After applying this change, I observed that it's becoming significantly
faster to navigate between desktop and mobile pages during back and
forward navigations.
BUG=707368
Review-Url: https://codereview.chromium.org/2811073005
Cr-Commit-Position: refs/heads/master@{#464830}
Committed: https://chromium.googlesource.com/chromium/src/+/b647f8362016d8e5b7a614e92b72331d070a32cf
Patch Set 1 #
Total comments: 15
Patch Set 2 : Address comments #
Total comments: 4
Patch Set 3 : Avoid calling setCustomUserAgent for every load #
Total comments: 4
Patch Set 4 : Address comments #Patch Set 5 : Remove unncessary if condition and add comments #
Messages
Total messages: 24 (6 generated)
liaoyuke@chromium.org changed reviewers: + eugenebut@chromium.org, kkhorimoto@chromium.org
Hey Eugene, Kurt, PTAL. Thank you very much!
CCing sdefresne@ for this: https://codereview.chromium.org/2811073005/diff/1/ios/chrome/browser/tabs/tab... https://codereview.chromium.org/2811073005/diff/1/ios/chrome/browser/tabs/tab.mm File ios/chrome/browser/tabs/tab.mm (left): https://codereview.chromium.org/2811073005/diff/1/ios/chrome/browser/tabs/tab... ios/chrome/browser/tabs/tab.mm:1372: [self.webController requirePageReconstruction]; Cool. Can you make this method private for CRWWebController? https://codereview.chromium.org/2811073005/diff/1/ios/web/web_state/ui/crw_we... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2811073005/diff/1/ios/web/web_state/ui/crw_we... ios/web/web_state/ui/crw_web_controller.mm:484: - (void)updateUserAgentForItem:(web::NavigationItem*)item updateWebViewUserAgentFromItem: ? https://codereview.chromium.org/2811073005/diff/1/ios/web/web_state/ui/crw_we... ios/web/web_state/ui/crw_web_controller.mm:1623: [_webView setCustomUserAgent:userAgent]; Are there any performance implications in calling this for every load? This call can potentially send a message to a separate process. https://codereview.chromium.org/2811073005/diff/1/ios/web/web_state/ui/crw_we... ios/web/web_state/ui/crw_web_controller.mm:2199: if (!self.currentNavItem) Old code with ternary could be faster https://codereview.chromium.org/2811073005/diff/1/ios/web/web_state/ui/crw_we... ios/web/web_state/ui/crw_web_controller.mm:2245: web::UserAgentType currentUserAgentType = item->GetUserAgentType(); This isn't "current", right? Current is one that is used in WKWebView. https://codereview.chromium.org/2811073005/diff/1/ios/web/web_state/ui/crw_we... ios/web/web_state/ui/crw_web_controller.mm:2250: NSString* currentUserAgent = base::SysUTF8ToNSString( ditto
PTAL. Thank you very much! https://codereview.chromium.org/2811073005/diff/1/ios/chrome/browser/tabs/tab.mm File ios/chrome/browser/tabs/tab.mm (left): https://codereview.chromium.org/2811073005/diff/1/ios/chrome/browser/tabs/tab... ios/chrome/browser/tabs/tab.mm:1372: [self.webController requirePageReconstruction]; On 2017/04/12 18:22:02, Eugene But wrote: > Cool. Can you make this method private for CRWWebController? Done. https://codereview.chromium.org/2811073005/diff/1/ios/web/web_state/ui/crw_we... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2811073005/diff/1/ios/web/web_state/ui/crw_we... ios/web/web_state/ui/crw_web_controller.mm:484: - (void)updateUserAgentForItem:(web::NavigationItem*)item On 2017/04/12 18:22:02, Eugene But wrote: > updateWebViewUserAgentFromItem: ? Thanks! that's even better, and after looking at the caller place, I think it makes more sense to pass in two items instead of an item and a user agent type. https://codereview.chromium.org/2811073005/diff/1/ios/web/web_state/ui/crw_we... ios/web/web_state/ui/crw_web_controller.mm:1623: [_webView setCustomUserAgent:userAgent]; On 2017/04/12 18:22:02, Eugene But wrote: > Are there any performance implications in calling this for every load? This call > can potentially send a message to a separate process. That is a very good question. I'm not sure how WKWebView works, is it no-op if |userAgent| is the same as WKWebView's current user agent? Calling this for every load is simple and clean, so as long as [_webView setCustomUserAgent:userAgent]; is not causing any noticeable delay, I think it worths the overhead, WDYT? https://codereview.chromium.org/2811073005/diff/1/ios/web/web_state/ui/crw_we... ios/web/web_state/ui/crw_web_controller.mm:2199: if (!self.currentNavItem) On 2017/04/12 18:22:02, Eugene But wrote: > Old code with ternary could be faster Done. https://codereview.chromium.org/2811073005/diff/1/ios/web/web_state/ui/crw_we... ios/web/web_state/ui/crw_web_controller.mm:2245: web::UserAgentType currentUserAgentType = item->GetUserAgentType(); On 2017/04/12 18:22:02, Eugene But wrote: > This isn't "current", right? Current is one that is used in WKWebView. Done. https://codereview.chromium.org/2811073005/diff/1/ios/web/web_state/ui/crw_we... ios/web/web_state/ui/crw_web_controller.mm:2250: NSString* currentUserAgent = base::SysUTF8ToNSString( On 2017/04/12 18:22:02, Eugene But wrote: > ditto Done.
https://codereview.chromium.org/2811073005/diff/20001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2811073005/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:487: // Requires that the next load rebuild the UIWebView. This is expensive, and s/UIWebView/WKWebView. https://codereview.chromium.org/2811073005/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:1629: NSString* userAgent = base::SysUTF8ToNSString( Can we check whether the current NavigationItem has a different UA type than the previous before setting the property? Or just call |-updateWebViewUserAgentFromItem:toItem|, which already has this logic?
PTAL @ the last patch. Thank you very much! https://codereview.chromium.org/2811073005/diff/20001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2811073005/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:487: // Requires that the next load rebuild the UIWebView. This is expensive, and On 2017/04/12 19:42:28, kkhorimoto_ wrote: > s/UIWebView/WKWebView. maybe web view? https://codereview.chromium.org/2811073005/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:1629: NSString* userAgent = base::SysUTF8ToNSString( On 2017/04/12 19:42:27, kkhorimoto_ wrote: > Can we check whether the current NavigationItem has a different UA type than the > previous before setting the property? Or just call > |-updateWebViewUserAgentFromItem:toItem|, which already has this logic? How about we directly compare web view's current customUserAgent to |userAgent|, and only call setCustomUserAgent if it's different?
https://codereview.chromium.org/2811073005/diff/1/ios/web/web_state/ui/crw_we... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2811073005/diff/1/ios/web/web_state/ui/crw_we... ios/web/web_state/ui/crw_web_controller.mm:1623: [_webView setCustomUserAgent:userAgent]; On 2017/04/12 19:17:59, liaoyuke wrote: > On 2017/04/12 18:22:02, Eugene But wrote: > > Are there any performance implications in calling this for every load? This > call > > can potentially send a message to a separate process. > > That is a very good question. I'm not sure how WKWebView works, is it no-op if > |userAgent| is the same as WKWebView's current user agent? WKWebView code is open source, so it should not be hard to check. https://codereview.chromium.org/2811073005/diff/40001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2811073005/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:484: - (void)updateWebViewUserAgentIfChanged:(web::UserAgentType)userAgentType; From "Coding Guidelines for Cocoa": Make the word before the argument describe the argument. IfChanged is unnecessary and it is just implied. https://codereview.chromium.org/2811073005/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:2245: if (![userAgent isEqualToString:[_webView customUserAgent]]) Is there guarantee that customUserAgent is not nil? Passing nil to isEqualToString: will crash the app.
PTAL. Thank you very much! https://codereview.chromium.org/2811073005/diff/40001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2811073005/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:484: - (void)updateWebViewUserAgentIfChanged:(web::UserAgentType)userAgentType; On 2017/04/13 00:34:06, Eugene But wrote: > From "Coding Guidelines for Cocoa": Make the word before the argument describe > the argument. IfChanged is unnecessary and it is just implied. Done. https://codereview.chromium.org/2811073005/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:2245: if (![userAgent isEqualToString:[_webView customUserAgent]]) On 2017/04/13 00:34:06, Eugene But wrote: > Is there guarantee that customUserAgent is not nil? Passing nil to > isEqualToString: will crash the app. Yes, in BuildWKWebView, we always set the customUserAgent property, but that's a good point, so I'm swapping |userAgent| and [_webView customUserAgent] in case the implementation of BuildWKWebView changes in the future.
lgtm
FYI: there is still unanswered comment
https://codereview.chromium.org/2811073005/diff/1/ios/web/web_state/ui/crw_we... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2811073005/diff/1/ios/web/web_state/ui/crw_we... ios/web/web_state/ui/crw_web_controller.mm:1623: [_webView setCustomUserAgent:userAgent]; On 2017/04/13 00:34:06, Eugene But wrote: > On 2017/04/12 19:17:59, liaoyuke wrote: > > On 2017/04/12 18:22:02, Eugene But wrote: > > > Are there any performance implications in calling this for every load? This > > call > > > can potentially send a message to a separate process. > > > > That is a very good question. I'm not sure how WKWebView works, is it no-op if > > |userAgent| is the same as WKWebView's current user agent? > WKWebView code is open source, so it should not be hard to check. Sorry, forgot to reply. I was only able to find the header file of WKWebView, but not source file, so I still don't know if there is performance implications.
https://codereview.chromium.org/2811073005/diff/1/ios/web/web_state/ui/crw_we... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2811073005/diff/1/ios/web/web_state/ui/crw_we... ios/web/web_state/ui/crw_web_controller.mm:1623: [_webView setCustomUserAgent:userAgent]; On 2017/04/13 23:03:53, liaoyuke wrote: > On 2017/04/13 00:34:06, Eugene But wrote: > > On 2017/04/12 19:17:59, liaoyuke wrote: > > > On 2017/04/12 18:22:02, Eugene But wrote: > > > > Are there any performance implications in calling this for every load? > This > > > call > > > > can potentially send a message to a separate process. > > > > > > That is a very good question. I'm not sure how WKWebView works, is it no-op > if > > > |userAgent| is the same as WKWebView's current user agent? > > WKWebView code is open source, so it should not be hard to check. > > Sorry, forgot to reply. I was only able to find the header file of WKWebView, > but not source file, so I still don't know if there is performance implications. WKWebView source code: https://trac.webkit.org/browser/webkit/trunk/Source/WebKit2/UIProcess/API/Cocoa More info on WebKit: https://webkit.org/getting-the-code/
On 2017/04/14 00:19:53, Eugene But wrote: > https://codereview.chromium.org/2811073005/diff/1/ios/web/web_state/ui/crw_we... > File ios/web/web_state/ui/crw_web_controller.mm (right): > > https://codereview.chromium.org/2811073005/diff/1/ios/web/web_state/ui/crw_we... > ios/web/web_state/ui/crw_web_controller.mm:1623: [_webView > setCustomUserAgent:userAgent]; > On 2017/04/13 23:03:53, liaoyuke wrote: > > On 2017/04/13 00:34:06, Eugene But wrote: > > > On 2017/04/12 19:17:59, liaoyuke wrote: > > > > On 2017/04/12 18:22:02, Eugene But wrote: > > > > > Are there any performance implications in calling this for every load? > > This > > > > call > > > > > can potentially send a message to a separate process. > > > > > > > > That is a very good question. I'm not sure how WKWebView works, is it > no-op > > if > > > > |userAgent| is the same as WKWebView's current user agent? > > > WKWebView code is open source, so it should not be hard to check. > > > > Sorry, forgot to reply. I was only able to find the header file of WKWebView, > > but not source file, so I still don't know if there is performance > implications. > WKWebView source code: > https://trac.webkit.org/browser/webkit/trunk/Source/WebKit2/UIProcess/API/Cocoa > > More info on WebKit: > https://webkit.org/getting-the-code/ Thanks for sharing the link! I didn't find it because I didn't realize WKWebView is part of WebKit :( WKWebView: 999 - (void)setCustomUserAgent:(NSString *)customUserAgent 1000 { 1001 _page->setCustomUserAgent(customUserAgent); 1002 } Here the _page is WebPageProxy WebPageProxy: 2327 void WebPageProxy::setUserAgent(const String& userAgent) 2328 { 2329 if (m_userAgent == userAgent) 2330 return; 2331 m_userAgent = userAgent; 2332 2333 if (!isValid()) 2334 return; 2335 m_process->send(Messages::WebPage::SetUserAgent(m_userAgent), m_pageID); 2336 } So, it seems that it's no-op when the new user agent is the same as the current user agent, otherwise, it will send a message. However, I would still prefer our current implementation over blindly setCustomUserAgent because the the no-op is an implementation detail, and it may change any time in the future, so current implementation is more robust. WDYT?
On 2017/04/14 05:53:42, liaoyuke wrote: > On 2017/04/14 00:19:53, Eugene But wrote: > > > https://codereview.chromium.org/2811073005/diff/1/ios/web/web_state/ui/crw_we... > > File ios/web/web_state/ui/crw_web_controller.mm (right): > > > > > https://codereview.chromium.org/2811073005/diff/1/ios/web/web_state/ui/crw_we... > > ios/web/web_state/ui/crw_web_controller.mm:1623: [_webView > > setCustomUserAgent:userAgent]; > > On 2017/04/13 23:03:53, liaoyuke wrote: > > > On 2017/04/13 00:34:06, Eugene But wrote: > > > > On 2017/04/12 19:17:59, liaoyuke wrote: > > > > > On 2017/04/12 18:22:02, Eugene But wrote: > > > > > > Are there any performance implications in calling this for every load? > > > This > > > > > call > > > > > > can potentially send a message to a separate process. > > > > > > > > > > That is a very good question. I'm not sure how WKWebView works, is it > > no-op > > > if > > > > > |userAgent| is the same as WKWebView's current user agent? > > > > WKWebView code is open source, so it should not be hard to check. > > > > > > Sorry, forgot to reply. I was only able to find the header file of > WKWebView, > > > but not source file, so I still don't know if there is performance > > implications. > > WKWebView source code: > > > https://trac.webkit.org/browser/webkit/trunk/Source/WebKit2/UIProcess/API/Cocoa > > > > More info on WebKit: > > https://webkit.org/getting-the-code/ > > Thanks for sharing the link! I didn't find it because I didn't realize WKWebView > is part of WebKit :( > > WKWebView: > 999 - (void)setCustomUserAgent:(NSString *)customUserAgent > 1000 { > 1001 _page->setCustomUserAgent(customUserAgent); > 1002 } > > Here the _page is WebPageProxy > WebPageProxy: > 2327 void WebPageProxy::setUserAgent(const String& userAgent) > 2328 { > 2329 if (m_userAgent == userAgent) > 2330 return; > 2331 m_userAgent = userAgent; > 2332 > 2333 if (!isValid()) > 2334 return; > 2335 m_process->send(Messages::WebPage::SetUserAgent(m_userAgent), > m_pageID); > 2336 } > > So, it seems that it's no-op when the new user agent is the same as the current > user agent, otherwise, it will send a message. > > However, I would still prefer our current implementation over blindly > setCustomUserAgent because the the no-op is an implementation detail, and it may > change any time in the future, so current implementation is more robust. WDYT? Thank you for checking. Current implementation is not faster and that unlikely will change (Apple will not break other apps on purpose). customUserAgent setter can be changed by accident, so is the getter. So I don't think this check will help us in any way. WDYT?
Patchset #5 (id:80001) has been deleted
On 2017/04/14 13:48:09, Eugene But wrote: > On 2017/04/14 05:53:42, liaoyuke wrote: > > On 2017/04/14 00:19:53, Eugene But wrote: > > > > > > https://codereview.chromium.org/2811073005/diff/1/ios/web/web_state/ui/crw_we... > > > File ios/web/web_state/ui/crw_web_controller.mm (right): > > > > > > > > > https://codereview.chromium.org/2811073005/diff/1/ios/web/web_state/ui/crw_we... > > > ios/web/web_state/ui/crw_web_controller.mm:1623: [_webView > > > setCustomUserAgent:userAgent]; > > > On 2017/04/13 23:03:53, liaoyuke wrote: > > > > On 2017/04/13 00:34:06, Eugene But wrote: > > > > > On 2017/04/12 19:17:59, liaoyuke wrote: > > > > > > On 2017/04/12 18:22:02, Eugene But wrote: > > > > > > > Are there any performance implications in calling this for every > load? > > > > This > > > > > > call > > > > > > > can potentially send a message to a separate process. > > > > > > > > > > > > That is a very good question. I'm not sure how WKWebView works, is it > > > no-op > > > > if > > > > > > |userAgent| is the same as WKWebView's current user agent? > > > > > WKWebView code is open source, so it should not be hard to check. > > > > > > > > Sorry, forgot to reply. I was only able to find the header file of > > WKWebView, > > > > but not source file, so I still don't know if there is performance > > > implications. > > > WKWebView source code: > > > > > > https://trac.webkit.org/browser/webkit/trunk/Source/WebKit2/UIProcess/API/Cocoa > > > > > > More info on WebKit: > > > https://webkit.org/getting-the-code/ > > > > Thanks for sharing the link! I didn't find it because I didn't realize > WKWebView > > is part of WebKit :( > > > > WKWebView: > > 999 - (void)setCustomUserAgent:(NSString *)customUserAgent > > 1000 { > > 1001 _page->setCustomUserAgent(customUserAgent); > > 1002 } > > > > Here the _page is WebPageProxy > > WebPageProxy: > > 2327 void WebPageProxy::setUserAgent(const String& userAgent) > > 2328 { > > 2329 if (m_userAgent == userAgent) > > 2330 return; > > 2331 m_userAgent = userAgent; > > 2332 > > 2333 if (!isValid()) > > 2334 return; > > 2335 m_process->send(Messages::WebPage::SetUserAgent(m_userAgent), > > m_pageID); > > 2336 } > > > > So, it seems that it's no-op when the new user agent is the same as the > current > > user agent, otherwise, it will send a message. > > > > However, I would still prefer our current implementation over blindly > > setCustomUserAgent because the the no-op is an implementation detail, and it > may > > change any time in the future, so current implementation is more robust. WDYT? > Thank you for checking. Current implementation is not faster and that unlikely > will change (Apple will not break other apps on purpose). customUserAgent setter > can be changed by accident, so is the getter. So I don't think this check will > help us in any way. WDYT? That sounds reasonable to me! PTAL @ the latest patch. Thanks for the detailed review, really appreciate it!
This is great! lgtm
The CQ bit was checked by liaoyuke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kkhorimoto@chromium.org Link to the patchset: https://codereview.chromium.org/2811073005/#ps100001 (title: "Remove unncessary if condition and add comments")
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": 100001, "attempt_start_ts": 1492212703321380,
"parent_rev": "ab15adbe306f64451bb0debb09f67d54f19d597f", "commit_rev":
"b647f8362016d8e5b7a614e92b72331d070a32cf"}
Message was sent while issue was closed.
Description was changed from ========== Replace requirePageReconstruction with setCustomUserAgent WKWebView provides a property: customUserAgent that we can use to change the user agent associated with the web view, which means that we don't have to requirePageConstruction anymore when user agent changes. After applying this change, I observed that it's becoming significantly faster to navigate between desktop and mobile pages during back and forward navigations. BUG=707368 ========== to ========== Replace requirePageReconstruction with setCustomUserAgent WKWebView provides a property: customUserAgent that we can use to change the user agent associated with the web view, which means that we don't have to requirePageConstruction anymore when user agent changes. After applying this change, I observed that it's becoming significantly faster to navigate between desktop and mobile pages during back and forward navigations. BUG=707368 Review-Url: https://codereview.chromium.org/2811073005 Cr-Commit-Position: refs/heads/master@{#464830} Committed: https://chromium.googlesource.com/chromium/src/+/b647f8362016d8e5b7a614e92b72... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001) as https://chromium.googlesource.com/chromium/src/+/b647f8362016d8e5b7a614e92b72...
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:100001) has been created in https://codereview.chromium.org/2820983003/ by rohitrao@chromium.org. The reason for reverting is: Seeing crashes in EG tests: https://build.chromium.org/p/chromium.fyi/builders/EarlGreyiOS/builds/26550. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
