Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2568713003:
[Android] Switch AwContentsTest to EmbeddedTestServer (Closed)
|
Created:
4 years ago by shenghuazhang Modified:
4 years ago CC:
android-webview-reviews_chromium.org, chromium-reviews, agrieve+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Android] Switch AwContentsTest to EmbeddedTestServer
Replace deprecated package org.apache.http.Header and
org.apache.http.HttpRequest in AwContentsTest.java with
EmbeddedTestServer.
* Merge crrev.com/2580573002 into this cl.
[Android] Modify loadURL and make a copy of additionalHttpHeaders
* Move maybeStripDoubleQuotes to AwTestBase for multiple use in each tests.
Also refactor AcceptLanguageTest.java which uses it.
To avoid the raw additionalHttpHeaders to be modified in later usage, make a copy of it when pass it into params as extra headers.
BUG=488194
Committed: https://crrev.com/0e1205a90260eee71b8cab3cb48f90b0eca67cc7
Cr-Commit-Position: refs/heads/master@{#438902}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Change header string to avoid 'referer' #
Total comments: 1
Patch Set 3 : Add referer head checking #
Total comments: 4
Patch Set 4 : John comment #
Total comments: 10
Patch Set 5 : Change referer header to be an url #
Total comments: 5
Patch Set 6 : Selim comment #
Total comments: 2
Messages
Total messages: 38 (12 generated)
shenghuazhang@chromium.org changed reviewers: + jbudorick@chromium.org
Referer header raw string is "None", needs to discuss it. https://codereview.chromium.org/2568713003/diff/1/android_webview/javatests/s... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java (right): https://codereview.chromium.org/2568713003/diff/1/android_webview/javatests/s... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:480: String referer = getReferer(JSUtils.executeJavaScriptAndWaitForResult(this, awContents, In this way, the 'JSUtils.executeJavaScriptAndWaitForResult' result raw string is expected "foo" but is "None" actually. Should the url be set as different way? I also tried url with "/set-header": String url = testServer.getURL("/set-header?Referer: foo"); (and loadUrlSync without the extraHeaders), and the raw string result would be "Referer: foo". But I doubt if it's correct since the test case name is 'testCanInjectHeaders', so that the extra test header needs to be embedded when loads, but not to be set initially by getURL()?
Due to Referer header would be set separately and removed from header (link below), change the extra header string instead of 'Referer'. https://codesearch.chromium.org/chromium/src/android_webview/java/src/org/chr...
https://codereview.chromium.org/2568713003/diff/20001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java (left): https://codereview.chromium.org/2568713003/diff/20001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:460: for (Map.Entry<String, String> value : extraHeaders.entrySet()) { This is checking that everything in extraHeaders was in the request. What's in extraHeaders by the time we get here?
Add referer head and check equal to 'None'.
non-owner lgtm w/ nits https://codereview.chromium.org/2568713003/diff/40001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java (right): https://codereview.chromium.org/2568713003/diff/40001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:449: * @param raw String containing the raw foo header nit: these should not reference the above "foo" example. https://codereview.chromium.org/2568713003/diff/40001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:484: // If injecting a Referer extra header with calling loadUrl, Referer header should be nit: s/with/when/
Patchset #4 (id:60001) has been deleted
shenghuazhang@chromium.org changed reviewers: + boliu@chromium.org
@boliu Hi Bo, can you please review this CL as the owner? Thanks~ https://codereview.chromium.org/2568713003/diff/40001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java (right): https://codereview.chromium.org/2568713003/diff/40001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:449: * @param raw String containing the raw foo header On 2016/12/14 16:14:45, jbudorick wrote: > nit: these should not reference the above "foo" example. Done. https://codereview.chromium.org/2568713003/diff/40001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:484: // If injecting a Referer extra header with calling loadUrl, Referer header should be On 2016/12/14 16:14:45, jbudorick wrote: > nit: s/with/when/ Done.
https://codereview.chromium.org/2568713003/diff/80001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java (right): https://codereview.chromium.org/2568713003/diff/80001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:447: * "bar" this comment is pretty confusing, I can see this is stripping double quotes, if any but comment here says result still contains quotes? just call this "maybeStripDoubleQuotes", and then don't need the javadoc https://codereview.chromium.org/2568713003/diff/80001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:484: // If injecting a Referer extra header when calling loadUrl, Referer header should be this doesn't seem to match the previous test behavior?
https://codereview.chromium.org/2568713003/diff/80001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java (right): https://codereview.chromium.org/2568713003/diff/80001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:484: // If injecting a Referer extra header when calling loadUrl, Referer header should be On 2016/12/14 19:31:02, boliu wrote: > this doesn't seem to match the previous test behavior? oh, AwContents actually modifies that map, so previous test never actually verified what happened to the referer.. Hmm.... should dig a bit what happened here
https://codereview.chromium.org/2568713003/diff/80001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java (right): https://codereview.chromium.org/2568713003/diff/80001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:484: // If injecting a Referer extra header when calling loadUrl, Referer header should be On 2016/12/14 19:37:27, boliu wrote: > On 2016/12/14 19:31:02, boliu wrote: > > this doesn't seem to match the previous test behavior? > > oh, AwContents actually modifies that map, so previous test never actually > verified what happened to the referer.. > > Hmm.... should dig a bit what happened here Ok, so there are multiple things broken here. AwContents should not modify the map passed to it, so it should make a copy first here: https://cs.chromium.org/chromium/src/android_webview/java/src/org/chromium/an... That will fix the test not actually verifying setting the referer header. But then apparently setting the referer actually is broken, as evident with the "None" check here. sgurun tells me that's a real production bug, so yay, refactoring a test found a real bug. File a crbug I guess.
boliu@chromium.org changed reviewers: + sgurun@chromium.org
+sgurun
btw, why is the cc list empty? webview changes should auto cc android-webview-reviews
On 2016/12/14 20:00:57, boliu wrote: > btw, why is the cc list empty? webview changes should auto cc > android-webview-reviews was this manually removed? otherwise it would be scary to miss stuff here.
On 2016/12/14 23:43:06, sgurun wrote: > On 2016/12/14 20:00:57, boliu wrote: > > btw, why is the cc list empty? webview changes should auto cc > > android-webview-reviews > > was this manually removed? otherwise it would be scary to miss stuff here. I think if you upload with bypass-hooks, then it doesn't process watchlists. It's never a guarantee though, you can always hand modify the cc list after upload.
Description was changed from ========== [Android] Switch AwContentsTest to EmbeddedTestServer Replace deprecated package org.apache.http.Header and org.apache.http.HttpRequest in AwContentsTest.java with EmbeddedTestServer. BUG=488194 ========== to ========== [Android] Switch AwContentsTest to EmbeddedTestServer Replace deprecated package org.apache.http.Header and org.apache.http.HttpRequest in AwContentsTest.java with EmbeddedTestServer. BUG=488194 ==========
On 2016/12/14 23:46:02, boliu wrote: > On 2016/12/14 23:43:06, sgurun wrote: > > On 2016/12/14 20:00:57, boliu wrote: > > > btw, why is the cc list empty? webview changes should auto cc > > > android-webview-reviews > > > > was this manually removed? otherwise it would be scary to miss stuff here. > > I think if you upload with bypass-hooks, then it doesn't process watchlists. > It's never a guarantee though, you can always hand modify the cc list after > upload. shenghuazhang@ have you filed a bug on the WebView for this? sorry if I missed.
@sgurun Sure I will file a bug for it. @boliu For the cc list, the android-webview is removed manually, and I have added them back. Sorry for the confusion! https://codereview.chromium.org/2568713003/diff/80001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java (right): https://codereview.chromium.org/2568713003/diff/80001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:447: * "bar" On 2016/12/14 19:31:02, boliu wrote: > this comment is pretty confusing, I can see this is stripping double quotes, if > any > > but comment here says result still contains quotes? > > just call this "maybeStripDoubleQuotes", and then don't need the javadoc Yes this makes sense. In the case the method simply helps the result string to get rid of the quotes. Will modify it in the next patch. https://codereview.chromium.org/2568713003/diff/80001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:484: // If injecting a Referer extra header when calling loadUrl, Referer header should be On 2016/12/14 19:58:40, boliu wrote: > On 2016/12/14 19:37:27, boliu wrote: > > On 2016/12/14 19:31:02, boliu wrote: > > > this doesn't seem to match the previous test behavior? > > > > oh, AwContents actually modifies that map, so previous test never actually > > verified what happened to the referer.. > > > > Hmm.... should dig a bit what happened here > > Ok, so there are multiple things broken here. > > AwContents should not modify the map passed to it, so it should make a copy > first here: > https://cs.chromium.org/chromium/src/android_webview/java/src/org/chromium/an... > That will fix the test not actually verifying setting the referer header. > > But then apparently setting the referer actually is broken, as evident with the > "None" check here. sgurun tells me that's a real production bug, so yay, > refactoring a test found a real bug. File a crbug I guess. So the correct behavior should be a real header value string check here instead of "None". I will file a bug for it.
Description was changed from ========== [Android] Switch AwContentsTest to EmbeddedTestServer Replace deprecated package org.apache.http.Header and org.apache.http.HttpRequest in AwContentsTest.java with EmbeddedTestServer. BUG=488194 ========== to ========== [Android] Switch AwContentsTest to EmbeddedTestServer Replace deprecated package org.apache.http.Header and org.apache.http.HttpRequest in AwContentsTest.java with EmbeddedTestServer. * Merge crrev.com/2580573002 into this cl. [Android] Modify loadURL and make a copy of additionalHttpHeaders To avoid the raw additionalHttpHeaders to be modified in later usage, make a copy of it when pass it into params as extra headers. BUG=488194 ==========
not really looked at it yet, but two comments. https://codereview.chromium.org/2568713003/diff/80001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java (right): https://codereview.chromium.org/2568713003/diff/80001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:37: import org.chromium.net.test.util.TestWebServer; so are we going to keep both here in this test? https://codereview.chromium.org/2568713003/diff/80001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:467: getAwSettingsOnUiThread(awContents).setJavaScriptEnabled(true); hava a shortcut for this: enableJavaScriptOnUiThread(mAwContents)
Thanks for the reviews! https://codereview.chromium.org/2568713003/diff/80001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java (right): https://codereview.chromium.org/2568713003/diff/80001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:37: import org.chromium.net.test.util.TestWebServer; On 2016/12/15 02:12:44, sgurun wrote: > so are we going to keep both here in this test? Discussed with @jbudorick. It would be better to use one, but converting from TestWebServer is lower priority. Will file another bug for it. https://codereview.chromium.org/2568713003/diff/80001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:467: getAwSettingsOnUiThread(awContents).setJavaScriptEnabled(true); On 2016/12/15 02:12:44, sgurun wrote: > hava a shortcut for this: enableJavaScriptOnUiThread(mAwContents) Thanks for the better case. Done. https://codereview.chromium.org/2568713003/diff/100001/android_webview/javate... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java (right): https://codereview.chromium.org/2568713003/diff/100001/android_webview/javate... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:472: extraHeaders.put("Referer", "http://www.example.com/"); Verified the Referer header behavior with @sgurun. The Referer header value should set as an url e.g. http://www.example.com/ instead of a random string, otherwise the content would filter the referer header out if not an webpage address, which shouldn't be a bug.
Merged crrev.com/2580573002 into this cl in patch set 5.
lgtm
https://codereview.chromium.org/2568713003/diff/100001/android_webview/javate... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java (right): https://codereview.chromium.org/2568713003/diff/100001/android_webview/javate... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:59: private static final Pattern MAYBE_QUOTED_STRING = Pattern.compile("^(\"?)(.*)\\1$"); Let's not repeat the same pattern over and over in our tests. move this to AwTestBase. Also update AcceptLanguageTest and remove it from there. https://codereview.chromium.org/2568713003/diff/100001/android_webview/javate... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:437: private String maybeStripDoubleQuotes(String raw) { move this to AwTestBase and make protected. Update AcceptLanguageTest. getAcceptLanguages() to use your new method.
Description was changed from ========== [Android] Switch AwContentsTest to EmbeddedTestServer Replace deprecated package org.apache.http.Header and org.apache.http.HttpRequest in AwContentsTest.java with EmbeddedTestServer. * Merge crrev.com/2580573002 into this cl. [Android] Modify loadURL and make a copy of additionalHttpHeaders To avoid the raw additionalHttpHeaders to be modified in later usage, make a copy of it when pass it into params as extra headers. BUG=488194 ========== to ========== [Android] Switch AwContentsTest to EmbeddedTestServer Replace deprecated package org.apache.http.Header and org.apache.http.HttpRequest in AwContentsTest.java with EmbeddedTestServer. * Merge crrev.com/2580573002 into this cl. [Android] Modify loadURL and make a copy of additionalHttpHeaders * Move maybeStripDoubleQuotes to AwTestBase for multiple use in each tests. Also refactor AcceptLanguageTest.java which uses it. To avoid the raw additionalHttpHeaders to be modified in later usage, make a copy of it when pass it into params as extra headers. BUG=488194 ==========
Moved maybeStripDoubleQuotes to AwTestBase. Please review it again. https://codereview.chromium.org/2568713003/diff/100001/android_webview/javate... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java (right): https://codereview.chromium.org/2568713003/diff/100001/android_webview/javate... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:59: private static final Pattern MAYBE_QUOTED_STRING = Pattern.compile("^(\"?)(.*)\\1$"); On 2016/12/15 15:45:54, sgurun wrote: > Let's not repeat the same pattern over and over in our tests. > move this to AwTestBase. Also update AcceptLanguageTest and remove it from > there. Done. https://codereview.chromium.org/2568713003/diff/100001/android_webview/javate... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:437: private String maybeStripDoubleQuotes(String raw) { On 2016/12/15 15:45:54, sgurun wrote: > move this to AwTestBase and make protected. Update AcceptLanguageTest. > getAcceptLanguages() to use your new method. > Done. https://codereview.chromium.org/2568713003/diff/120001/android_webview/javate... File android_webview/javatests/src/org/chromium/android_webview/test/AcceptLanguageTest.java (right): https://codereview.chromium.org/2568713003/diff/120001/android_webview/javate... android_webview/javatests/src/org/chromium/android_webview/test/AcceptLanguageTest.java:68: return COMMA_AND_OPTIONAL_Q_VALUE.split(maybeStripDoubleQuotes(raw)); Refactor by using maybeStripDoubleQuotes. Didn't move method getAcceptLanguages into AwTestBase, since it is rarely used in other files.
lgtm, thanks https://codereview.chromium.org/2568713003/diff/120001/android_webview/javate... File android_webview/javatests/src/org/chromium/android_webview/test/AcceptLanguageTest.java (right): https://codereview.chromium.org/2568713003/diff/120001/android_webview/javate... android_webview/javatests/src/org/chromium/android_webview/test/AcceptLanguageTest.java:68: return COMMA_AND_OPTIONAL_Q_VALUE.split(maybeStripDoubleQuotes(raw)); On 2016/12/15 19:09:49, shenghuazhang wrote: > Refactor by using maybeStripDoubleQuotes. Didn't move method getAcceptLanguages > into AwTestBase, since it is rarely used in other files. yep, my comment for moving was not for this, either.
Thank you for all the reviews!
The CQ bit was checked by shenghuazhang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jbudorick@chromium.org, boliu@chromium.org Link to the patchset: https://codereview.chromium.org/2568713003/#ps120001 (title: "Selim comment")
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": 120001, "attempt_start_ts": 1481829729953230,
"parent_rev": "9f5772d39b002be5996c5e1b107141424717d01b", "commit_rev":
"b9fb8c7aa3d0dfddf5b76cdbfecf354883374334"}
Message was sent while issue was closed.
Description was changed from ========== [Android] Switch AwContentsTest to EmbeddedTestServer Replace deprecated package org.apache.http.Header and org.apache.http.HttpRequest in AwContentsTest.java with EmbeddedTestServer. * Merge crrev.com/2580573002 into this cl. [Android] Modify loadURL and make a copy of additionalHttpHeaders * Move maybeStripDoubleQuotes to AwTestBase for multiple use in each tests. Also refactor AcceptLanguageTest.java which uses it. To avoid the raw additionalHttpHeaders to be modified in later usage, make a copy of it when pass it into params as extra headers. BUG=488194 ========== to ========== [Android] Switch AwContentsTest to EmbeddedTestServer Replace deprecated package org.apache.http.Header and org.apache.http.HttpRequest in AwContentsTest.java with EmbeddedTestServer. * Merge crrev.com/2580573002 into this cl. [Android] Modify loadURL and make a copy of additionalHttpHeaders * Move maybeStripDoubleQuotes to AwTestBase for multiple use in each tests. Also refactor AcceptLanguageTest.java which uses it. To avoid the raw additionalHttpHeaders to be modified in later usage, make a copy of it when pass it into params as extra headers. BUG=488194 Review-Url: https://codereview.chromium.org/2568713003 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [Android] Switch AwContentsTest to EmbeddedTestServer Replace deprecated package org.apache.http.Header and org.apache.http.HttpRequest in AwContentsTest.java with EmbeddedTestServer. * Merge crrev.com/2580573002 into this cl. [Android] Modify loadURL and make a copy of additionalHttpHeaders * Move maybeStripDoubleQuotes to AwTestBase for multiple use in each tests. Also refactor AcceptLanguageTest.java which uses it. To avoid the raw additionalHttpHeaders to be modified in later usage, make a copy of it when pass it into params as extra headers. BUG=488194 Review-Url: https://codereview.chromium.org/2568713003 ========== to ========== [Android] Switch AwContentsTest to EmbeddedTestServer Replace deprecated package org.apache.http.Header and org.apache.http.HttpRequest in AwContentsTest.java with EmbeddedTestServer. * Merge crrev.com/2580573002 into this cl. [Android] Modify loadURL and make a copy of additionalHttpHeaders * Move maybeStripDoubleQuotes to AwTestBase for multiple use in each tests. Also refactor AcceptLanguageTest.java which uses it. To avoid the raw additionalHttpHeaders to be modified in later usage, make a copy of it when pass it into params as extra headers. BUG=488194 Committed: https://crrev.com/0e1205a90260eee71b8cab3cb48f90b0eca67cc7 Cr-Commit-Position: refs/heads/master@{#438902} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/0e1205a90260eee71b8cab3cb48f90b0eca67cc7 Cr-Commit-Position: refs/heads/master@{#438902} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
