Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2957423002:
Check page URL origin to determine JavaScript alert titles. (Closed)
|
Created:
3 years, 5 months ago by PL Modified:
3 years, 5 months ago Reviewers:
Eugene But (OOO till 7-30) CC:
chromium-reviews, marq+watch_chromium.org, ios-reviews+chrome_chromium.org, noyau+watch_chromium.org, ios-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionCheck page URL origin to determine JavaScript alert titles.
BUG=590815
Review-Url: https://codereview.chromium.org/2957423002
Cr-Commit-Position: refs/heads/master@{#483583}
Committed: https://chromium.googlesource.com/chromium/src/+/3ed4f536d098e75cf5f999fdc076bcc81bad39ab
Patch Set 1 #
Total comments: 10
Patch Set 2 : Review feedback and tests #Patch Set 3 : Add string comparison unit test #
Total comments: 6
Patch Set 4 : Review feedback: Make localizedTitle...: private, tweaks. #
Total comments: 2
Patch Set 5 : Small tweak #
Messages
Total messages: 23 (13 generated)
peterlaurens@chromium.org changed reviewers: + eugenebut@chromium.org
Hello! This is a tweak as discussed last week to the JavaScript alert title logic. I spent some time looking at updating the EGTests to include an alert triggered from an iframe with a different domain, however I could not get the embedded HTTP server to serve content from a different domain (always serves from 127.0.0.1 as the origin, even if the URL string we create in the test is different, e.g. http://x or http://y, everything is served from localhost). I'm guessing there's no way around this without hacking up DNS in some way under test so that we could use a different domain still point to localhost?). Thanks! https://codereview.chromium.org/2957423002/diff/1/ios/chrome/browser/ui/dialo... File ios/chrome/browser/ui/dialogs/dialog_presenter.mm (right): https://codereview.chromium.org/2957423002/diff/1/ios/chrome/browser/ui/dialo... ios/chrome/browser/ui/dialogs/dialog_presenter.mm:176: mainFrameURL:webState->GetLastCommittedURL()]; Is GetLastCommittedURL() the right URL to use here for the main frame? Thanks!
Thank you for fixing this! Is it possible to add a test to dialog_presenter_unittest? On 2017/06/28 23:18:34, PL wrote: > Hello! > > This is a tweak as discussed last week to the JavaScript alert title logic. > > I spent some time looking at updating the EGTests to include an alert triggered > from an iframe with a different domain, however I could not get the embedded > HTTP server to serve content from a different domain (always serves from > 127.0.0.1 as the origin, even if the URL string we create in the test is > different, e.g. http://x or http://y, everything is served from localhost). I'm > guessing there's no way around this without hacking up DNS in some way under > test so that we could use a different domain still point to localhost?). web::HttpServer::MakeUrl should give you URL, which you could use to extract hostname. https://codereview.chromium.org/2957423002/diff/1/ios/chrome/browser/ui/dialo... File ios/chrome/browser/ui/dialogs/dialog_presenter.h (right): https://codereview.chromium.org/2957423002/diff/1/ios/chrome/browser/ui/dialo... ios/chrome/browser/ui/dialogs/dialog_presenter.h:83: + (NSString*)localizedTitleForJavaScriptAlertFromPage:(const GURL&)pageURL; Do we even need this method? It is only used in EG tests, which actually goes against testing best practices and defines back doors only for tests: go/unit-test-practices?cl=head#public-apis https://codereview.chromium.org/2957423002/diff/1/ios/chrome/browser/ui/dialo... ios/chrome/browser/ui/dialogs/dialog_presenter.h:85: // Create an title for the alert base on the URL (|pageURL|), and its s/Create/Creates https://codereview.chromium.org/2957423002/diff/1/ios/chrome/browser/ui/dialo... ios/chrome/browser/ui/dialogs/dialog_presenter.h:88: + (NSString*)localizedTitleForJavaScriptAlertFromPage:(const GURL&)pageURL Do we even need this method in the public interface? https://codereview.chromium.org/2957423002/diff/1/ios/chrome/browser/ui/dialo... File ios/chrome/browser/ui/dialogs/dialog_presenter.mm (right): https://codereview.chromium.org/2957423002/diff/1/ios/chrome/browser/ui/dialo... ios/chrome/browser/ui/dialogs/dialog_presenter.mm:366: bool is_same_origin_as_main_frame = s/is_same_origin_as_main_frame/sameOriginAsMainFrame
Thanks! Couple quick replies first: On 2017/06/29 01:00:01, Eugene But wrote: > Thank you for fixing this! Is it possible to add a test to > dialog_presenter_unittest? Of course! I was so focussed on the EG Test problem I didn't consider a unit test, this should be very simple, will be in the next patch I upload. > > > On 2017/06/28 23:18:34, PL wrote: > > Hello! > > > > This is a tweak as discussed last week to the JavaScript alert title logic. > > > > I spent some time looking at updating the EGTests to include an alert > triggered > > from an iframe with a different domain, however I could not get the embedded > > HTTP server to serve content from a different domain (always serves from > > 127.0.0.1 as the origin, even if the URL string we create in the test is > > different, e.g. http://x or http://y, everything is served from localhost). > I'm > > guessing there's no way around this without hacking up DNS in some way under > > test so that we could use a different domain still point to localhost?). > web::HttpServer::MakeUrl should give you URL, which you could use to extract > hostname. Unfortunately MakeUrl always makes a URL with the host 127.0.0.1, so the browser will not see this as a different host for the iframe. > > https://codereview.chromium.org/2957423002/diff/1/ios/chrome/browser/ui/dialo... > File ios/chrome/browser/ui/dialogs/dialog_presenter.h (right): > > https://codereview.chromium.org/2957423002/diff/1/ios/chrome/browser/ui/dialo... > ios/chrome/browser/ui/dialogs/dialog_presenter.h:83: + > (NSString*)localizedTitleForJavaScriptAlertFromPage:(const GURL&)pageURL; > Do we even need this method? It is only used in EG tests, which actually goes > against testing best practices and defines back doors only for tests: > > go/unit-test-practices?cl=head#public-apis > > https://codereview.chromium.org/2957423002/diff/1/ios/chrome/browser/ui/dialo... > ios/chrome/browser/ui/dialogs/dialog_presenter.h:85: // Create an title for the > alert base on the URL (|pageURL|), and its > s/Create/Creates > > https://codereview.chromium.org/2957423002/diff/1/ios/chrome/browser/ui/dialo... > ios/chrome/browser/ui/dialogs/dialog_presenter.h:88: + > (NSString*)localizedTitleForJavaScriptAlertFromPage:(const GURL&)pageURL > Do we even need this method in the public interface? > > https://codereview.chromium.org/2957423002/diff/1/ios/chrome/browser/ui/dialo... > File ios/chrome/browser/ui/dialogs/dialog_presenter.mm (right): > > https://codereview.chromium.org/2957423002/diff/1/ios/chrome/browser/ui/dialo... > ios/chrome/browser/ui/dialogs/dialog_presenter.mm:366: bool > is_same_origin_as_main_frame = > s/is_same_origin_as_main_frame/sameOriginAsMainFrame
>> Unfortunately MakeUrl always makes a URL with the host 127.0.0.1, so the browser will not see this as a different host for the iframe. Sorry, I misunderstood your question. You were asking how to write EG test for this particular test case. I don't think we need an EG test for that.
Ok! Thanks Eugene, This change adds unit tests that check the alert title for expectations. PTAL! https://codereview.chromium.org/2957423002/diff/1/ios/chrome/browser/ui/dialo... File ios/chrome/browser/ui/dialogs/dialog_presenter.h (right): https://codereview.chromium.org/2957423002/diff/1/ios/chrome/browser/ui/dialo... ios/chrome/browser/ui/dialogs/dialog_presenter.h:83: + (NSString*)localizedTitleForJavaScriptAlertFromPage:(const GURL&)pageURL; On 2017/06/29 01:00:01, Eugene But wrote: > Do we even need this method? It is only used in EG tests, which actually goes > against testing best practices and defines back doors only for tests: > > go/unit-test-practices?cl=head#public-apis Sure thing! I did think about this, and whether it would be worse to change tests in three places or keep the old API pointing to the new API. https://codereview.chromium.org/2957423002/diff/1/ios/chrome/browser/ui/dialo... ios/chrome/browser/ui/dialogs/dialog_presenter.h:85: // Create an title for the alert base on the URL (|pageURL|), and its On 2017/06/29 01:00:01, Eugene But wrote: > s/Create/Creates Arg, done! https://codereview.chromium.org/2957423002/diff/1/ios/chrome/browser/ui/dialo... ios/chrome/browser/ui/dialogs/dialog_presenter.h:88: + (NSString*)localizedTitleForJavaScriptAlertFromPage:(const GURL&)pageURL On 2017/06/29 01:00:01, Eugene But wrote: > Do we even need this method in the public interface? It's only going to be used by the EGTest, but I think this has to stay so those tests can run (or have this in an internal header rather than the private one?). This does seem to break the idea of exposing API to tests only, but this was the case before this CL. https://codereview.chromium.org/2957423002/diff/1/ios/chrome/browser/ui/dialo... File ios/chrome/browser/ui/dialogs/dialog_presenter.mm (right): https://codereview.chromium.org/2957423002/diff/1/ios/chrome/browser/ui/dialo... ios/chrome/browser/ui/dialogs/dialog_presenter.mm:366: bool is_same_origin_as_main_frame = On 2017/06/29 01:00:01, Eugene But wrote: > s/is_same_origin_as_main_frame/sameOriginAsMainFrame Thanks! I'm not sure how long it will take me to get this convention right :-O.
The CQ bit was checked by peterlaurens@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2957423002/diff/1/ios/chrome/browser/ui/dialo... File ios/chrome/browser/ui/dialogs/dialog_presenter.h (right): https://codereview.chromium.org/2957423002/diff/1/ios/chrome/browser/ui/dialo... ios/chrome/browser/ui/dialogs/dialog_presenter.h:88: + (NSString*)localizedTitleForJavaScriptAlertFromPage:(const GURL&)pageURL On 2017/06/29 20:02:21, PL wrote: > On 2017/06/29 01:00:01, Eugene But wrote: > > Do we even need this method in the public interface? > > It's only going to be used by the EGTest, but I think this has to stay so those > tests can run (or have this in an internal header rather than the private one?). > > This does seem to break the idea of exposing API to tests only, but this was the > case before this CL. Can we just hardcode the title in those tests? A agree that encapsulation violation was not introduced by this CL, but it's something that should be relatively easy to fix. If it's hard to fix, then let's leave it as it is. https://codereview.chromium.org/2957423002/diff/40001/ios/chrome/browser/ui/d... File ios/chrome/browser/ui/dialogs/dialog_presenter_unittest.mm (right): https://codereview.chromium.org/2957423002/diff/40001/ios/chrome/browser/ui/d... ios/chrome/browser/ui/dialogs/dialog_presenter_unittest.mm:118: EXPECT_FALSE([same_origin_title length] == 0); Is it possible to check the title in this test? Tests are always run with english locale, so localization should not be a problem. https://codereview.chromium.org/2957423002/diff/40001/ios/chrome/browser/ui/d... ios/chrome/browser/ui/dialogs/dialog_presenter_unittest.mm:131: EXPECT_FALSE([different_origin_title length] == 0); Instead of this and the next "EXPECT" call you can do as follows: EXPECT_NSEQ(l10n_util::GetNSString(IDS_JAVASCRIPT_MESSAGEBOX_TITLE_NONSTANDARD_URL_IFRAME), different_origin_title) https://codereview.chromium.org/2957423002/diff/40001/ios/chrome/browser/ui/d... ios/chrome/browser/ui/dialogs/dialog_presenter_unittest.mm:136: EXPECT_FALSE([same_origin_title isEqualToString:different_origin_title]); Is it possible to drop this "EXPECT" in favor of comparing |same_origin_title| to hardcoded string?
Thanks Eugene! Was fairly easy to make localizedTitleForJavaScriptAlertFromPage:mainFrameURL: private. PTAL! - Peter https://codereview.chromium.org/2957423002/diff/40001/ios/chrome/browser/ui/d... File ios/chrome/browser/ui/dialogs/dialog_presenter_unittest.mm (right): https://codereview.chromium.org/2957423002/diff/40001/ios/chrome/browser/ui/d... ios/chrome/browser/ui/dialogs/dialog_presenter_unittest.mm:118: EXPECT_FALSE([same_origin_title length] == 0); On 2017/06/29 20:35:10, Eugene But wrote: > Is it possible to check the title in this test? Tests are always run with > english locale, so localization should not be a problem. Done! https://codereview.chromium.org/2957423002/diff/40001/ios/chrome/browser/ui/d... ios/chrome/browser/ui/dialogs/dialog_presenter_unittest.mm:131: EXPECT_FALSE([different_origin_title length] == 0); On 2017/06/29 20:35:10, Eugene But wrote: > Instead of this and the next "EXPECT" call you can do as follows: > > EXPECT_NSEQ(l10n_util::GetNSString(IDS_JAVASCRIPT_MESSAGEBOX_TITLE_NONSTANDARD_URL_IFRAME), > different_origin_title) Done! https://codereview.chromium.org/2957423002/diff/40001/ios/chrome/browser/ui/d... ios/chrome/browser/ui/dialogs/dialog_presenter_unittest.mm:136: EXPECT_FALSE([same_origin_title isEqualToString:different_origin_title]); On 2017/06/29 20:35:10, Eugene But wrote: > Is it possible to drop this "EXPECT" in favor of comparing |same_origin_title| > to hardcoded string? Done! You're right, this one won't be needed any more.
lgtm! Thank you for fixing this. https://codereview.chromium.org/2957423002/diff/60001/ios/chrome/browser/ui/d... File ios/chrome/browser/ui/dialogs/dialog_presenter_unittest.mm (right): https://codereview.chromium.org/2957423002/diff/60001/ios/chrome/browser/ui/d... ios/chrome/browser/ui/dialogs/dialog_presenter_unittest.mm:122: EXPECT_NSEQ(same_origin_title, expected_title); EXPECT_NSEQ takes first argument as expected result and second as actual result https://codereview.chromium.org/2957423002/diff/60001/ios/chrome/browser/ui/d... ios/chrome/browser/ui/dialogs/dialog_presenter_unittest.mm:137: EXPECT_NSEQ(different_origin_title, expected_title); ditto
The CQ bit was checked by peterlaurens@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by peterlaurens@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eugenebut@chromium.org Link to the patchset: https://codereview.chromium.org/2957423002/#ps80001 (title: "Small tweak")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1498786282198760,
"parent_rev": "21219ce3bbda77f391197e1ac17cd3ba01645f60", "commit_rev":
"3ed4f536d098e75cf5f999fdc076bcc81bad39ab"}
Message was sent while issue was closed.
Description was changed from ========== Check page URL origin to determine JavaScript alert titles. BUG=590815 ========== to ========== Check page URL origin to determine JavaScript alert titles. BUG=590815 Review-Url: https://codereview.chromium.org/2957423002 Cr-Commit-Position: refs/heads/master@{#483583} Committed: https://chromium.googlesource.com/chromium/src/+/3ed4f536d098e75cf5f999fdc076... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/3ed4f536d098e75cf5f999fdc076... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
