Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2684313002:
[WebViewLayoutTest] Implement a fallback mechanism and re-enable testWebViewIncludedStableInterface… (Closed)
|
Created:
3 years, 10 months ago by timvolodine Modified:
3 years, 10 months ago CC:
chromium-reviews, android-webview-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[WebViewLayoutTest] Implement a fallback mechanism and re-enable testWebViewIncludedStableInterfaces.
Implement a 'fallback' mechanism for the selection of appropriate
global interfaces file to compare with. The approach checks a number
of files in the list to determine which one to use. Due to the nature
of rebaselining in blink the suitable virtual/stable/webexposed/
global-interface-listing-expected.txt can be located in a number of
places and can change dynamically.
This patch also enables the testWebViewIncludedStableInterfaces test
which was previously disabled due to the above issue.
BUG=683153, 506603, 497861
NOTRY=true
Review-Url: https://codereview.chromium.org/2684313002
Cr-Original-Commit-Position: refs/heads/master@{#450743}
Committed: https://chromium.googlesource.com/chromium/src/+/4965734c3a73e6fc608071eba416e9af6b83555d
Review-Url: https://codereview.chromium.org/2684313002
Cr-Commit-Position: refs/heads/master@{#451100}
Committed: https://chromium.googlesource.com/chromium/src/+/558119e51b5c263536897d096ee5e62f852cddca
Patch Set 1 #
Total comments: 2
Patch Set 2 : address comments, add more explanation and link #Patch Set 3 : fallback order #Patch Set 4 : fix bots #
Messages
Total messages: 35 (23 generated)
timvolodine@chromium.org changed reviewers: + tobiasjs@chromium.org, torne@chromium.org
LGTM, but if possible add more to the comment explaining this: https://codereview.chromium.org/2684313002/diff/1/android_webview/tools/syste... File android_webview/tools/system_webview_shell/layout_tests/src/org/chromium/webview_shell/test/WebViewLayoutTest.java (right): https://codereview.chromium.org/2684313002/diff/1/android_webview/tools/syste... android_webview/tools/system_webview_shell/layout_tests/src/org/chromium/webview_shell/test/WebViewLayoutTest.java:51: // against a fallback approach is used. The order in the List below is important. Is the ordering here copied from some place in the rebaseline script or something? It's good to include the comment that it's important, but it would be better if you could actually describe why it's this order, because it's a bit surprising that windows comes before linux for webview :)
The CQ bit was checked by timvolodine@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 timvolodine@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...
Thanks for the review Torne, tried to address your comment below.. https://codereview.chromium.org/2684313002/diff/1/android_webview/tools/syste... File android_webview/tools/system_webview_shell/layout_tests/src/org/chromium/webview_shell/test/WebViewLayoutTest.java (right): https://codereview.chromium.org/2684313002/diff/1/android_webview/tools/syste... android_webview/tools/system_webview_shell/layout_tests/src/org/chromium/webview_shell/test/WebViewLayoutTest.java:51: // against a fallback approach is used. The order in the List below is important. On 2017/02/10 13:57:47, Torne wrote: > Is the ordering here copied from some place in the rebaseline script or > something? It's good to include the comment that it's important, but it would be > better if you could actually describe why it's this order, because it's a bit > surprising that windows comes before linux for webview :) I've added more explanation and included a pointer to the blink optimization script which does the magic.. There is even a picture as illustration: https://docs.google.com/drawings/d/1eGdsIKzJ2dxDDBbUaIABrN4aMLD1bqJTfyxNGZsTd... Also changed the order to look for linux before windows, currently it will fallback to windows anyway though due to baseline optimization.
OK, still LGTM
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 timvolodine@chromium.org
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 timvolodine@chromium.org
Description was changed from ========== [WebViewLayoutTest] Implement a fallback mechanism and re-enable testWebViewIncludedStableInterfaces. Implement a 'fallback' mechanism for the selection of appropriate global interfaces file to compare with. The approach checks a number of files in the list to determine which one to use. Due to the nature of rebaselining in blink the suitable virtual/stable/webexposed/ global-interface-listing-expected.txt can be located in a number of places and can change dynamically. This patch also enables the testWebViewIncludedStableInterfaces test which was previously disabled due to the above issue. BUG=683153 ========== to ========== [WebViewLayoutTest] Implement a fallback mechanism and re-enable testWebViewIncludedStableInterfaces. Implement a 'fallback' mechanism for the selection of appropriate global interfaces file to compare with. The approach checks a number of files in the list to determine which one to use. Due to the nature of rebaselining in blink the suitable virtual/stable/webexposed/ global-interface-listing-expected.txt can be located in a number of places and can change dynamically. This patch also enables the testWebViewIncludedStableInterfaces test which was previously disabled due to the above issue. BUG=683153,506603,497861 ==========
The CQ bit was checked by timvolodine@chromium.org
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": 40001, "attempt_start_ts": 1487175399551090,
"parent_rev": "03f2053328cb3ab19f5c5a0348cc3581576bbe49", "commit_rev":
"4965734c3a73e6fc608071eba416e9af6b83555d"}
Message was sent while issue was closed.
Description was changed from ========== [WebViewLayoutTest] Implement a fallback mechanism and re-enable testWebViewIncludedStableInterfaces. Implement a 'fallback' mechanism for the selection of appropriate global interfaces file to compare with. The approach checks a number of files in the list to determine which one to use. Due to the nature of rebaselining in blink the suitable virtual/stable/webexposed/ global-interface-listing-expected.txt can be located in a number of places and can change dynamically. This patch also enables the testWebViewIncludedStableInterfaces test which was previously disabled due to the above issue. BUG=683153,506603,497861 ========== to ========== [WebViewLayoutTest] Implement a fallback mechanism and re-enable testWebViewIncludedStableInterfaces. Implement a 'fallback' mechanism for the selection of appropriate global interfaces file to compare with. The approach checks a number of files in the list to determine which one to use. Due to the nature of rebaselining in blink the suitable virtual/stable/webexposed/ global-interface-listing-expected.txt can be located in a number of places and can change dynamically. This patch also enables the testWebViewIncludedStableInterfaces test which was previously disabled due to the above issue. BUG=683153,506603,497861 Review-Url: https://codereview.chromium.org/2684313002 Cr-Commit-Position: refs/heads/master@{#450743} Committed: https://chromium.googlesource.com/chromium/src/+/4965734c3a73e6fc608071eba416... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/4965734c3a73e6fc608071eba416...
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2694283007/ by wychen@chromium.org. The reason for reverting is: Break SystemWebViewShellLayoutTest.SystemWebViewShellLayoutTest on WebView bots https://uberchromegw.corp.google.com/i/chromium.android/builders/Android%20We... https://uberchromegw.corp.google.com/i/chromium.android/builders/Android%20We....
Message was sent while issue was closed.
Description was changed from ========== [WebViewLayoutTest] Implement a fallback mechanism and re-enable testWebViewIncludedStableInterfaces. Implement a 'fallback' mechanism for the selection of appropriate global interfaces file to compare with. The approach checks a number of files in the list to determine which one to use. Due to the nature of rebaselining in blink the suitable virtual/stable/webexposed/ global-interface-listing-expected.txt can be located in a number of places and can change dynamically. This patch also enables the testWebViewIncludedStableInterfaces test which was previously disabled due to the above issue. BUG=683153,506603,497861 Review-Url: https://codereview.chromium.org/2684313002 Cr-Commit-Position: refs/heads/master@{#450743} Committed: https://chromium.googlesource.com/chromium/src/+/4965734c3a73e6fc608071eba416... ========== to ========== [WebViewLayoutTest] Implement a fallback mechanism and re-enable testWebViewIncludedStableInterfaces. Implement a 'fallback' mechanism for the selection of appropriate global interfaces file to compare with. The approach checks a number of files in the list to determine which one to use. Due to the nature of rebaselining in blink the suitable virtual/stable/webexposed/ global-interface-listing-expected.txt can be located in a number of places and can change dynamically. This patch also enables the testWebViewIncludedStableInterfaces test which was previously disabled due to the above issue. BUG=683153,506603,497861 Review-Url: https://codereview.chromium.org/2684313002 Cr-Commit-Position: refs/heads/master@{#450743} Committed: https://chromium.googlesource.com/chromium/src/+/4965734c3a73e6fc608071eba416... ==========
The CQ bit was checked by timvolodine@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from torne@chromium.org Link to the patchset: https://codereview.chromium.org/2684313002/#ps60001 (title: "fix bots")
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
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_...)
Description was changed from ========== [WebViewLayoutTest] Implement a fallback mechanism and re-enable testWebViewIncludedStableInterfaces. Implement a 'fallback' mechanism for the selection of appropriate global interfaces file to compare with. The approach checks a number of files in the list to determine which one to use. Due to the nature of rebaselining in blink the suitable virtual/stable/webexposed/ global-interface-listing-expected.txt can be located in a number of places and can change dynamically. This patch also enables the testWebViewIncludedStableInterfaces test which was previously disabled due to the above issue. BUG=683153,506603,497861 Review-Url: https://codereview.chromium.org/2684313002 Cr-Commit-Position: refs/heads/master@{#450743} Committed: https://chromium.googlesource.com/chromium/src/+/4965734c3a73e6fc608071eba416... ========== to ========== [WebViewLayoutTest] Implement a fallback mechanism and re-enable testWebViewIncludedStableInterfaces. Implement a 'fallback' mechanism for the selection of appropriate global interfaces file to compare with. The approach checks a number of files in the list to determine which one to use. Due to the nature of rebaselining in blink the suitable virtual/stable/webexposed/ global-interface-listing-expected.txt can be located in a number of places and can change dynamically. This patch also enables the testWebViewIncludedStableInterfaces test which was previously disabled due to the above issue. BUG=683153,506603,497861 NOTRY=true Review-Url: https://codereview.chromium.org/2684313002 Cr-Commit-Position: refs/heads/master@{#450743} Committed: https://chromium.googlesource.com/chromium/src/+/4965734c3a73e6fc608071eba416... ==========
The CQ bit was checked by timvolodine@chromium.org
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": 60001, "attempt_start_ts": 1487277226264380,
"parent_rev": "0af976576759401f90ec4b391fe22a408570013c", "commit_rev":
"558119e51b5c263536897d096ee5e62f852cddca"}
Message was sent while issue was closed.
Description was changed from ========== [WebViewLayoutTest] Implement a fallback mechanism and re-enable testWebViewIncludedStableInterfaces. Implement a 'fallback' mechanism for the selection of appropriate global interfaces file to compare with. The approach checks a number of files in the list to determine which one to use. Due to the nature of rebaselining in blink the suitable virtual/stable/webexposed/ global-interface-listing-expected.txt can be located in a number of places and can change dynamically. This patch also enables the testWebViewIncludedStableInterfaces test which was previously disabled due to the above issue. BUG=683153,506603,497861 NOTRY=true Review-Url: https://codereview.chromium.org/2684313002 Cr-Commit-Position: refs/heads/master@{#450743} Committed: https://chromium.googlesource.com/chromium/src/+/4965734c3a73e6fc608071eba416... ========== to ========== [WebViewLayoutTest] Implement a fallback mechanism and re-enable testWebViewIncludedStableInterfaces. Implement a 'fallback' mechanism for the selection of appropriate global interfaces file to compare with. The approach checks a number of files in the list to determine which one to use. Due to the nature of rebaselining in blink the suitable virtual/stable/webexposed/ global-interface-listing-expected.txt can be located in a number of places and can change dynamically. This patch also enables the testWebViewIncludedStableInterfaces test which was previously disabled due to the above issue. BUG=683153,506603,497861 NOTRY=true Review-Url: https://codereview.chromium.org/2684313002 Cr-Original-Commit-Position: refs/heads/master@{#450743} Committed: https://chromium.googlesource.com/chromium/src/+/4965734c3a73e6fc608071eba416... Review-Url: https://codereview.chromium.org/2684313002 Cr-Commit-Position: refs/heads/master@{#451100} Committed: https://chromium.googlesource.com/chromium/src/+/558119e51b5c263536897d096ee5... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/558119e51b5c263536897d096ee5... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
