Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2858043004:
Support interstitials with no primary button (Closed)
|
Created:
3 years, 7 months ago by edwardjung Modified:
3 years, 7 months ago CC:
chromium-reviews, jam, darin-cc_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionSupport interstitials with no primary button.
Current use case will be for showing interstitials in WebView, where 'Back to safety' isn't available.
BUG=716096
Review-Url: https://codereview.chromium.org/2858043004
Cr-Commit-Position: refs/heads/master@{#470311}
Committed: https://chromium.googlesource.com/chromium/src/+/5290088f4545b02abf2d0d6064d685ee0c7b50e2
Patch Set 1 #
Total comments: 4
Patch Set 2 : Fix nits #Patch Set 3 : Update iOS chrome controller #Patch Set 4 : Fix CanGoBack definition #Patch Set 5 : Fix JS errors on other interstitials #Patch Set 6 : Fix iOSChromeControllerClient #Patch Set 7 : Fix iOS build #
Total comments: 2
Patch Set 8 : Fix nits #
Total comments: 3
Patch Set 9 : Fix iOS CanGoBack implementation #Messages
Total messages: 48 (28 generated)
Description was changed from ========== Safe Browsing interstitials UI changes to support WebView BUG=716096 ========== to ========== Safe Browsing interstitials UI changes to support WebView Allows hiding the 'Back to safety' button. BUG=716096 ==========
edwardjung@chromium.org changed reviewers: + felt@chromium.org
Here's a separate CL for just the front end change. I'll switch CL 2852333003 to the rename. Thanks.
looks pretty good, just some nits https://codereview.chromium.org/2858043004/diff/1/components/security_interst... File components/security_interstitials/content/security_interstitial_controller_client.cc (right): https://codereview.chromium.org/2858043004/diff/1/components/security_interst... components/security_interstitials/content/security_interstitial_controller_client.cc:102: // For testing, should be set in the WV controller. I'm confused by this comment https://codereview.chromium.org/2858043004/diff/1/components/security_interst... File components/security_interstitials/content/security_interstitial_controller_client.h (right): https://codereview.chromium.org/2858043004/diff/1/components/security_interst... components/security_interstitials/content/security_interstitial_controller_client.h:51: bool CanGoBack() override; nit: can you move this to be next to GoBack()?
The CQ bit was checked by edwardjung@chromium.org to run a CQ dry run
Thanks, fixed nits. https://codereview.chromium.org/2858043004/diff/1/components/security_interst... File components/security_interstitials/content/security_interstitial_controller_client.cc (right): https://codereview.chromium.org/2858043004/diff/1/components/security_interst... components/security_interstitials/content/security_interstitial_controller_client.cc:102: // For testing, should be set in the WV controller. On 2017/05/05 00:31:14, felt wrote: > I'm confused by this comment Wasn't sure whether this was going to be hard coded for the desktop. I've changed this. https://codereview.chromium.org/2858043004/diff/1/components/security_interst... File components/security_interstitials/content/security_interstitial_controller_client.h (right): https://codereview.chromium.org/2858043004/diff/1/components/security_interst... components/security_interstitials/content/security_interstitial_controller_client.h:51: bool CanGoBack() override; On 2017/05/05 00:31:14, felt wrote: > nit: can you move this to be next to GoBack()? Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by edwardjung@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
I'm also having an issue with in the tests. They all fail at this point: https://cs.chromium.org/chromium/src/content/browser/frame_host/navigator_imp... Have you encountered this warning before? Seems to be a hack from way back. Thanks
On 2017/05/05 11:49:43, edwardjung wrote: > I'm also having an issue with in the tests. They all fail at this point: > > https://cs.chromium.org/chromium/src/content/browser/frame_host/navigator_imp... > > Have you encountered this warning before? Seems to be a hack from way back. > > Thanks I've never seen this before. Is that what's causing the "Request not handled. Returning 404: /ssl/google.html" error? At what point is that being triggered? Has the interstitial been displayed already, or is this on closing? Can you add a NOTREACHED into that method and share the dump?
On 2017/05/05 15:14:53, felt wrote: > On 2017/05/05 11:49:43, edwardjung wrote: > > I'm also having an issue with in the tests. They all fail at this point: > > > > > https://cs.chromium.org/chromium/src/content/browser/frame_host/navigator_imp... > > > > Have you encountered this warning before? Seems to be a hack from way back. > > > > Thanks > > I've never seen this before. Is that what's causing the "Request not handled. > Returning 404: /ssl/google.html" error? > > At what point is that being triggered? Has the interstitial been displayed > already, or is this on closing? Can you add a NOTREACHED into that method and > share the dump? I can do that. The bot error output is: [7320:6216:0505/063835.242:WARNING:navigator_impl.cc(269)] Discarding message during interstitial. [7668:5728:0505/063835.593:ERROR:render_process_impl.cc(170)] WebFrame LEAKED 1 TIMES [7320:1716:0505/063835.601:ERROR:process_win.cc(140)] Unable to terminate process: Access is denied. (0x5) [7320:7004:0505/063835.633:WARNING:embedded_test_server.cc(219)] Request not handled. Returning 404: /favicon.ico
Additional output when running the tests locally and adding a NOTREACHED:
2017-05-05 18:26:53.855 browser_tests[22311:717247] NSWindow warning: adding an
unknown subview: <FullSizeContentView: 0x7f95ca2000d0>. Break on NSLog to debug.
2017-05-05 18:26:53.855 browser_tests[22311:717247] Call stack:
(
"+callStackSymbols disabled for performance reasons"
)
[22311:34307:0505/182654.394798:ERROR:test_database_manager.cc(58)] Not
implemented reached in virtual bool
safe_browsing::TestSafeBrowsingDatabaseManager::CheckResourceUrl(const GURL &,
safe_browsing::SafeBrowsingDatabaseManager::Client *)
[22311:775:0505/182654.764741:WARNING:navigator_impl.cc(270)] Discarding message
during interstitial.
[22311:34307:0505/182654.833877:ERROR:test_database_manager.cc(58)] Not
implemented reached in virtual bool
safe_browsing::TestSafeBrowsingDatabaseManager::CheckResourceUrl(const GURL &,
safe_browsing::SafeBrowsingDatabaseManager::Client *)
[22311:90379:0505/182654.873529:WARNING:embedded_test_server.cc(219)] Request
not handled. Returning 404: /favicon.ico
[ OK ]
SafeBrowsingBlockingPageBrowserTestWithThreatTypeAndIsolationSetting/SafeBrowsingBlockingPageBrowserTest.SecurityStateGoBackOnSubresourceInterstitial/0
(1295 ms)
[----------] 1 test from
SafeBrowsingBlockingPageBrowserTestWithThreatTypeAndIsolationSetting/SafeBrowsingBlockingPageBrowserTest
(1295 ms total)
[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (1295 ms total)
[ PASSED ] 1 test.
Additional output when running the tests locally and adding a NOTREACHED:
2017-05-05 18:26:53.855 browser_tests[22311:717247] NSWindow warning: adding an
unknown subview: <FullSizeContentView: 0x7f95ca2000d0>. Break on NSLog to debug.
2017-05-05 18:26:53.855 browser_tests[22311:717247] Call stack:
(
"+callStackSymbols disabled for performance reasons"
)
[22311:34307:0505/182654.394798:ERROR:test_database_manager.cc(58)] Not
implemented reached in virtual bool
safe_browsing::TestSafeBrowsingDatabaseManager::CheckResourceUrl(const GURL &,
safe_browsing::SafeBrowsingDatabaseManager::Client *)
[22311:775:0505/182654.764741:WARNING:navigator_impl.cc(270)] Discarding message
during interstitial.
[22311:34307:0505/182654.833877:ERROR:test_database_manager.cc(58)] Not
implemented reached in virtual bool
safe_browsing::TestSafeBrowsingDatabaseManager::CheckResourceUrl(const GURL &,
safe_browsing::SafeBrowsingDatabaseManager::Client *)
[22311:90379:0505/182654.873529:WARNING:embedded_test_server.cc(219)] Request
not handled. Returning 404: /favicon.ico
[ OK ]
SafeBrowsingBlockingPageBrowserTestWithThreatTypeAndIsolationSetting/SafeBrowsingBlockingPageBrowserTest.SecurityStateGoBackOnSubresourceInterstitial/0
(1295 ms)
[----------] 1 test from
SafeBrowsingBlockingPageBrowserTestWithThreatTypeAndIsolationSetting/SafeBrowsingBlockingPageBrowserTest
(1295 ms total)
[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (1295 ms total)
[ PASSED ] 1 test.
On 2017/05/05 17:37:12, edwardjung wrote: > Additional output when running the tests locally and adding a NOTREACHED: > > 2017-05-05 18:26:53.855 browser_tests[22311:717247] NSWindow warning: adding an > unknown subview: <FullSizeContentView: 0x7f95ca2000d0>. Break on NSLog to debug. > 2017-05-05 18:26:53.855 browser_tests[22311:717247] Call stack: > ( > "+callStackSymbols disabled for performance reasons" > ) Are you building a release build or a debug build? It doesn't seem to have printed the call stack. If you're building a release build, you'll want to build a debug build instead to get more info
> Are you building a release build or a debug build? It doesn't seem to have > printed the call stack. If you're building a release build, you'll want to build > a debug build instead to get more info Here's the full stack from the test: https://paste.googleplex.com/5013936281747456
The CQ bit was checked by edwardjung@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
PTAL, I believe I located the issue. I needed to set a hide_primary_button value in the data for the other interstitials as it was causing a JS error in the tests.
On 2017/05/07 13:43:24, edwardjung wrote: > PTAL, I believe I located the issue. I needed to set a hide_primary_button > value in the data for the other interstitials as it was causing a JS error in > the tests. Ohhh yeah, that should have been set everywhere already - I guess the missing value had been hidden by the badClock check that used to be there! Nice catch.
lgtm, but please make the title of the issue more specific (maybe something like, "Support interstitials with no primary button") https://codereview.chromium.org/2858043004/diff/120001/components/security_in... File components/security_interstitials/core/controller_client.h (right): https://codereview.chromium.org/2858043004/diff/120001/components/security_in... components/security_interstitials/core/controller_client.h:74: // Whether it is possible to go 'Back to safety'. Specifially for WebViews. Nit: Suggestion, leave off the last sentence. (I think we might end up wanting to use this elsewhere too to fix some bugs, there are other situations besides WebView where you can't actually 'go back'.)
Description was changed from ========== Safe Browsing interstitials UI changes to support WebView Allows hiding the 'Back to safety' button. BUG=716096 ========== to ========== Support interstitials with no primary button. Current use case will be for showing interstitials in WebView, where 'Back to safety' isn't available. BUG=716096 ==========
The CQ bit was checked by edwardjung@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
edwardjung@chromium.org changed reviewers: + rohitrao@chromium.org
Thanks! @rohitrao, PTAL at the iOS updates to: ios/chrome/browser/interstitials/ios_chrome_controller_client.mm ios/chrome/browser/interstitials/ios_chrome_controller_client.h https://codereview.chromium.org/2858043004/diff/120001/components/security_in... File components/security_interstitials/core/controller_client.h (right): https://codereview.chromium.org/2858043004/diff/120001/components/security_in... components/security_interstitials/core/controller_client.h:74: // Whether it is possible to go 'Back to safety'. Specifially for WebViews. On 2017/05/07 14:00:39, felt wrote: > Nit: Suggestion, leave off the last sentence. (I think we might end up wanting > to use this elsewhere too to fix some bugs, there are other situations besides > WebView where you can't actually 'go back'.) Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2858043004/diff/140001/ios/chrome/browser/int... File ios/chrome/browser/interstitials/ios_chrome_controller_client.mm (right): https://codereview.chromium.org/2858043004/diff/140001/ios/chrome/browser/int... ios/chrome/browser/interstitials/ios_chrome_controller_client.mm:49: return true; Is this meant to always be true, even if the underlying WebState cannot go back? The //content implementation delegates to WebContents -- perhaps it's worth having some documentation explaining why iOS behaves differently?
https://codereview.chromium.org/2858043004/diff/140001/ios/chrome/browser/int... File ios/chrome/browser/interstitials/ios_chrome_controller_client.mm (right): https://codereview.chromium.org/2858043004/diff/140001/ios/chrome/browser/int... ios/chrome/browser/interstitials/ios_chrome_controller_client.mm:49: return true; On 2017/05/09 10:38:40, rohitrao (ping after 24h) wrote: > Is this meant to always be true, even if the underlying WebState cannot go back? > The //content implementation delegates to WebContents -- perhaps it's worth > having some documentation explaining why iOS behaves differently? This was added primarily for webview but you're right it should be a check here, is there an equivalent check in the WebState for this?
https://codereview.chromium.org/2858043004/diff/140001/ios/chrome/browser/int... File ios/chrome/browser/interstitials/ios_chrome_controller_client.mm (right): https://codereview.chromium.org/2858043004/diff/140001/ios/chrome/browser/int... ios/chrome/browser/interstitials/ios_chrome_controller_client.mm:49: return true; On 2017/05/09 10:38:40, rohitrao (ping after 24h) wrote: > Is this meant to always be true, even if the underlying WebState cannot go back? > The //content implementation delegates to WebContents -- perhaps it's worth > having some documentation explaining why iOS behaves differently? I found the equivalent and implemented here.
ios/ lgtm, thanks!
The CQ bit was checked by edwardjung@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by edwardjung@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from felt@chromium.org Link to the patchset: https://codereview.chromium.org/2858043004/#ps160001 (title: "Fix iOS CanGoBack implementation")
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": 160001, "attempt_start_ts": 1494337060904490,
"parent_rev": "4cfab296b9f2ec80a0c908d9f97a32b1ce70b361", "commit_rev":
"5290088f4545b02abf2d0d6064d685ee0c7b50e2"}
Message was sent while issue was closed.
Description was changed from ========== Support interstitials with no primary button. Current use case will be for showing interstitials in WebView, where 'Back to safety' isn't available. BUG=716096 ========== to ========== Support interstitials with no primary button. Current use case will be for showing interstitials in WebView, where 'Back to safety' isn't available. BUG=716096 Review-Url: https://codereview.chromium.org/2858043004 Cr-Commit-Position: refs/heads/master@{#470311} Committed: https://chromium.googlesource.com/chromium/src/+/5290088f4545b02abf2d0d6064d6... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/5290088f4545b02abf2d0d6064d6... |
