Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
964753002:
Test PostMessage to popup window (Closed)
|
Created:
5 years, 10 months ago by hush (inactive) Modified:
5 years, 9 months ago Reviewers:
sgurun-gerrit only CC:
chromium-reviews, android-webview-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTest PostMessage to popup window
BUG=460998
Committed: https://crrev.com/51c1620185396384cfe6e2f4e3359e57c6431600
Cr-Commit-Position: refs/heads/master@{#318591}
Committed: https://crrev.com/d4f192eb2d33e96a0705e8a81c39a1056c4eb0d3
Cr-Commit-Position: refs/heads/master@{#318721}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : test utils #
Total comments: 38
Patch Set 4 : comments #
Total comments: 4
Patch Set 5 : simplify JS #Patch Set 6 : #Patch Set 7 : Fix popup window test #Patch Set 8 : rebase #
Messages
Total messages: 26 (7 generated)
hush@chromium.org changed reviewers: + sgurun@chromium.org
PTAL
On 2015/02/27 03:04:50, hush wrote: > PTAL I will look at it tomorrow in detail, but I prefer extracting common parts in tests to a helper/utility class rather then copying.
PTAL ps3
I have some comments, but good work. thanks for helping me out! https://codereview.chromium.org/964753002/diff/40001/android_webview/javatest... File android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java (right): https://codereview.chromium.org/964753002/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:768: private static final String POPUP_MESSAGE = "The message from popup sent via port"; no need to be verbose. set the value to "from_popup" https://codereview.chromium.org/964753002/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:772: + " function createPopup() {" indent function by 4 to be consistent with other code in this file https://codereview.chromium.org/964753002/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:776: + " pop.postMessage(e.data, '*', e.ports);" indent prperly. https://codereview.chromium.org/964753002/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:779: + " window.addEventListener('message', relayMessage, false);" use onmessage = function(e) { popupWindow.postMessage(BLA) } which is simpler https://codereview.chromium.org/964753002/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:783: + "<body>" I don't think the body serves any purpose. remove. https://codereview.chromium.org/964753002/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:789: private static final String POPUP1 = "<!DOCTYPE html><html>" name it as POPUP1_PAGE https://codereview.chromium.org/964753002/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:789: private static final String POPUP1 = "<!DOCTYPE html><html>" actually why not use the ECHO_PAGE script for that. https://codereview.chromium.org/964753002/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:791: + "onmessage = function(e) {" indent by 4 https://codereview.chromium.org/964753002/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:797: + "This is the popup html" get rid of body. https://codereview.chromium.org/964753002/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:802: private static final String POPUP2 = "<!DOCTYPE html><html>" name as POPUP2_PAGE https://codereview.chromium.org/964753002/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:803: + "<title>popup</title>" get rid of title. https://codereview.chromium.org/964753002/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:811: + "This is the popup html" get rid of body. https://codereview.chromium.org/964753002/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:821: MAIN_PAGE_FOR_POPUP_TEST, POPUP1, "/popup.html", "createPopup()"); use a const for "popup.html" and in js code you refer to. https://codereview.chromium.org/964753002/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:836: drop empty line https://codereview.chromium.org/964753002/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:841: drop empty line https://codereview.chromium.org/964753002/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:867: drop empty line https://codereview.chromium.org/964753002/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:872: drop empty line https://codereview.chromium.org/964753002/diff/40001/android_webview/javatest... File android_webview/javatests/src/org/chromium/android_webview/test/util/PopupTestUtils.java (right): https://codereview.chromium.org/964753002/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/util/PopupTestUtils.java:19: public class PopupTestUtils { move both tests to awtestbase, which keeps utility methods shared between tests. then you can simplify the large parameter sets easily. https://codereview.chromium.org/964753002/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/util/PopupTestUtils.java:26: final String triggerScript) throws Throwable { I think making all the parameters final is inconsistent wrt rest of java code we have (also makes it a lot less readable when there is 4 lines of parameters)
https://codereview.chromium.org/964753002/diff/40001/android_webview/javatest... File android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java (right): https://codereview.chromium.org/964753002/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:768: private static final String POPUP_MESSAGE = "The message from popup sent via port"; On 2015/02/27 20:30:19, sgurun wrote: > no need to be verbose. set the value to "from_popup" Done. https://codereview.chromium.org/964753002/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:772: + " function createPopup() {" On 2015/02/27 20:30:19, sgurun wrote: > indent function by 4 to be consistent with other code in this file Done. https://codereview.chromium.org/964753002/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:776: + " pop.postMessage(e.data, '*', e.ports);" On 2015/02/27 20:30:19, sgurun wrote: > indent prperly. Done. https://codereview.chromium.org/964753002/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:779: + " window.addEventListener('message', relayMessage, false);" On 2015/02/27 20:30:19, sgurun wrote: > use onmessage = function(e) { > popupWindow.postMessage(BLA) > } > which is simpler Done. https://codereview.chromium.org/964753002/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:783: + "<body>" On 2015/02/27 20:30:19, sgurun wrote: > I don't think the body serves any purpose. remove. Done. https://codereview.chromium.org/964753002/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:789: private static final String POPUP1 = "<!DOCTYPE html><html>" On 2015/02/27 20:30:19, sgurun wrote: > actually why not use the ECHO_PAGE script for that. Done. https://codereview.chromium.org/964753002/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:791: + "onmessage = function(e) {" On 2015/02/27 20:30:19, sgurun wrote: > indent by 4 Done. https://codereview.chromium.org/964753002/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:802: private static final String POPUP2 = "<!DOCTYPE html><html>" On 2015/02/27 20:30:19, sgurun wrote: > name as POPUP2_PAGE I named it POPUP_PAGE_WITH_IFRAME https://codereview.chromium.org/964753002/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:803: + "<title>popup</title>" On 2015/02/27 20:30:19, sgurun wrote: > get rid of title. Done. https://codereview.chromium.org/964753002/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:811: + "This is the popup html" On 2015/02/27 20:30:19, sgurun wrote: > get rid of body. Done. https://codereview.chromium.org/964753002/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:821: MAIN_PAGE_FOR_POPUP_TEST, POPUP1, "/popup.html", "createPopup()"); On 2015/02/27 20:30:19, sgurun wrote: > use a const for "popup.html" and in js code you refer to. Done. https://codereview.chromium.org/964753002/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:821: MAIN_PAGE_FOR_POPUP_TEST, POPUP1, "/popup.html", "createPopup()"); On 2015/02/27 20:30:19, sgurun wrote: > use a const for "popup.html" and in js code you refer to. Done. https://codereview.chromium.org/964753002/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:836: On 2015/02/27 20:30:19, sgurun wrote: > drop empty line Done. https://codereview.chromium.org/964753002/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:836: On 2015/02/27 20:30:19, sgurun wrote: > drop empty line Done. https://codereview.chromium.org/964753002/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:841: On 2015/02/27 20:30:19, sgurun wrote: > drop empty line Done. https://codereview.chromium.org/964753002/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:867: On 2015/02/27 20:30:19, sgurun wrote: > drop empty line Done. https://codereview.chromium.org/964753002/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:872: On 2015/02/27 20:30:19, sgurun wrote: > drop empty line Done. https://codereview.chromium.org/964753002/diff/40001/android_webview/javatest... File android_webview/javatests/src/org/chromium/android_webview/test/util/PopupTestUtils.java (right): https://codereview.chromium.org/964753002/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/util/PopupTestUtils.java:19: public class PopupTestUtils { On 2015/02/27 20:30:20, sgurun wrote: > move both tests to awtestbase, which keeps utility methods shared between tests. > then you can simplify the large parameter sets easily. Talked in person. Let's just move this class into AwTestBase since we use awTestBase in the util methods so many times https://codereview.chromium.org/964753002/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/util/PopupTestUtils.java:26: final String triggerScript) throws Throwable { On 2015/02/27 20:30:19, sgurun wrote: > I think making all the parameters final is inconsistent wrt rest of java code we > have (also makes it a lot less readable when there is 4 lines of parameters) I just removed the unnecessary finals
lgtm with 2 nits. https://codereview.chromium.org/964753002/diff/60001/android_webview/javatest... File android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java (right): https://codereview.chromium.org/964753002/diff/60001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:776: + " return function (e) {" nit: indent -4 https://codereview.chromium.org/964753002/diff/60001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:794: + "<iframe src='" + IFRAME_URL + "'></iframe>" nit: I think you can move this and the next line up, next to body.
https://codereview.chromium.org/964753002/diff/60001/android_webview/javatest... File android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java (right): https://codereview.chromium.org/964753002/diff/60001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:776: + " return function (e) {" On 2015/02/28 01:18:23, sgurun wrote: > nit: indent -4 Done. https://codereview.chromium.org/964753002/diff/60001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:794: + "<iframe src='" + IFRAME_URL + "'></iframe>" On 2015/02/28 01:18:23, sgurun wrote: > nit: I think you can move this and the next line up, next to body. Done.
The CQ bit was checked by hush@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sgurun@chromium.org Link to the patchset: https://codereview.chromium.org/964753002/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/964753002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
On 2015/02/28 02:15:41, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...) Sorry about that... I only ran PostMessageTest but not the PopupWindowTest locally.... Fixed in PS 7.
On 2015/02/28 02:53:47, hush wrote: > On 2015/02/28 02:15:41, I haz the power (commit-bot) wrote: > > Try jobs failed on following builders: > > android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...) > > Sorry about that... I only ran PostMessageTest but not the PopupWindowTest > locally.... > > Fixed in PS 7. lgtm, assuming fixed :)
The CQ bit was checked by hush@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/964753002/110001
Message was sent while issue was closed.
Committed patchset #7 (id:110001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/51c1620185396384cfe6e2f4e3359e57c6431600 Cr-Commit-Position: refs/heads/master@{#318591}
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:110001) has been created in https://codereview.chromium.org/971433002/ by hush@chromium.org. The reason for reverting is: This landed soon after https://crrev.com/65b9392697befefd009dbe73d75dfe66894d814c which changed the signature of setWebEventHandler As a result, it broke the linux build bot. http://build.chromium.org/p/chromium.linux/builders/Android%20Arm64%20Builder....
Sorry the original commit (PS7) mid-air collided with your CL here: https://codereview.chromium.org/961393002/ and it broke the build bot... Re-opening the CL for uploading PS8.
The CQ bit was checked by sgurun@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sgurun@chromium.org Link to the patchset: https://codereview.chromium.org/964753002/#ps130001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/964753002/130001
Message was sent while issue was closed.
Committed patchset #8 (id:130001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/d4f192eb2d33e96a0705e8a81c39a1056c4eb0d3 Cr-Commit-Position: refs/heads/master@{#318721} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
