WebApk notifications broken pre M63 |
|||||||||||
Issue descriptionhttps://chromium-review.googlesource.com/667002 included a change to our aidl which specifies communication between chrome and webapk. Unfortunately, the new method was added in the middle and not at the end which is incompatible with aidl serialization format. All methods must be added at the end! This was new in M63: https://chromiumdash.appspot.com/commit/3e297acb7ca8a3c36b73028eea3352405a27cd3e We need to ship a new runtime library that corrects the order before this goes stable! otherwise, changes to webapk will break. in terms of impact to chrome, chrome was already going to rev from runtime library 1 to 2 so there's nothing new there. The only additional risk is to webapk notifications but this will fix that (so I think the risk is ok). cmasso: happy to discuss. Peter will have a fix in trunk tomorrow.
,
Nov 27 2017
,
Nov 27 2017
This bug requires manual review: We are only 7 days from stable. Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), gkihumba@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Nov 28 2017
Merge approved based on my offline conversation with pkotwicz@. Please make sure to verify this change on branch 3239 after merging it.
,
Nov 28 2017
Ping!
,
Nov 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/014db0dac917190372705920bf76b219a33e3b7f commit 014db0dac917190372705920bf76b219a33e3b7f Author: Peter Kotwicz <pkotwicz@chromium.org> Date: Tue Nov 28 20:22:32 2017 [Android WebAPK] Fix WebAPK runtime library AIDL order The compiler assigns to each method in the AIDL file a numeric identifier based on the method's order in the AIDL file. https://chromium-review.googlesource.com/c/chromium/src/+/667002 added a method in the middle of the AIDL file. As a result, for: - Chrome which has been compiled against the AIDL prior to the CL (pre 63.0.3227.0) - a WebAPK which has been compiled against the AIDL after the CL A call from Chrome for IWebApkApi#notifyNotification() attempts to run notificationPermissionEnabled() in the WebAPK BUG= 788003 R=​yfriedman TBR=rsesek (The CL only changes the order of the methods in the aidl file) Change-Id: Ia6912e53322e2622cd15dd318b47b9f451b29c00 Reviewed-on: https://chromium-review.googlesource.com/788182 Commit-Queue: Peter Kotwicz <pkotwicz@chromium.org> Reviewed-by: Yaron Friedman <yfriedman@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#519026}(cherry picked from commit 7f5f5c05a0f798b06b54fce08215c2fce924e5da) Reviewed-on: https://chromium-review.googlesource.com/794350 Reviewed-by: Peter Kotwicz <pkotwicz@chromium.org> Cr-Commit-Position: refs/branch-heads/3239@{#588} Cr-Branched-From: adb61db19020ed8ecee5e91b1a0ea4c924ae2988-refs/heads/master@{#508578} [modify] https://crrev.com/014db0dac917190372705920bf76b219a33e3b7f/chrome/android/webapk/libs/runtime_library/src/org/chromium/webapk/lib/runtime_library/IWebApkApi.aidl [modify] https://crrev.com/014db0dac917190372705920bf76b219a33e3b7f/chrome/android/webapk/libs/runtime_library_version.gni
,
Nov 28 2017
Repro steps for test team: 1) On Android O, Install attached WebAPK 2) Install Chrome Canary 63.0.3226.0 3) Launch WebAPK (with name "Notifications") The WebAPK should launch full screen 4) If prompted to choose a browser, choose "Chrome Canary" 5) Tap the "Display the notification" button on the web page (If the button is disabled, refresh the web page by swiping down) 6) Notice that an android notification is shown 7) Long press on the notification. Notice that the notification was sent by the "Notifications" app (and not from Chrome) Do steps 3 - 5 with Chrome Canary 63.0.3227.0 installed. Expected: The notification is shown as before Actual: The notification is not shown This bug should be fixed with my CL. The bug is present in Chrome Canary 63.0.3239.8 I am unsure if there is a newer official build yet.
,
Nov 29 2017
Verified fix with steps and apk from #7 in 63.0.3239.71 and 64.0.3279.0. Note: This issue didn't could be reproduced only with this version of WebApk. Didn't repro for recently installed WebApks .
,
Nov 29 2017
Marking it fixed. According to offline discussion with pkotwicz@, there is no more work needed here. |
|||||||||||
►
Sign in to add a comment |
|||||||||||
Comment 1 by bugdroid1@chromium.org
, Nov 23 2017