Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2068733002:
Fix manifest reference to cache invalidation alarm receiver. (Closed)
DescriptionFix manifest reference to cache invalidation alarm receiver.
A mistake happened when rolling in invalidation code to chromium, where the
package name of some classes were changed, but the android manifest for Chrome was
not updated.
The code in question is relates to a path that is not required for basic
usage of invalidation, but fixing the reference makes the invalidation code less
prone to failure.
This was discovered through a Proguard note about a missing class, so by updating
the reference, the note also disappears.
BUG=619937
Committed: https://crrev.com/0a191afe97691594f1cee9e33cdeb5b7a2f51e87
Cr-Commit-Position: refs/heads/master@{#404182}
Patch Set 1 #Patch Set 2 : Removing instead of renaming #Patch Set 3 : Back to first patch #Messages
Total messages: 34 (16 generated)
Description was changed from ========== Changed client2 to client BUG= ========== to ========== This line of the manifest seems broken. Removing it since it shouldn't matter whether it's there or not. com.google.ipc.invalidation.external.client2.contrib.AndroidListener$AlarmReceiver There does seem to be a very similarly named class - com.google.ipc.invalidation.external.client.contrib.AndroidListener Note - client2 vs client. BUG=619937 ==========
Description was changed from ========== This line of the manifest seems broken. Removing it since it shouldn't matter whether it's there or not. com.google.ipc.invalidation.external.client2.contrib.AndroidListener$AlarmReceiver There does seem to be a very similarly named class - com.google.ipc.invalidation.external.client.contrib.AndroidListener Note - client2 vs client. BUG=619937 ========== to ========== This line of the manifest seems broken. Removing it since it shouldn't matter whether it's there or not. com.google.ipc.invalidation.external.client2.contrib.AndroidListener$AlarmReceiver There does seem to be a very similarly named class - com.google.ipc.invalidation.external.client.contrib.AndroidListener Note - client2 vs client. BUG=619937 ==========
smaier@chromium.org changed reviewers: + tedchoc@chromium.org
tedchoc@chromium.org - can you look at AndroidManifest.xml
tedchoc@chromium.org changed reviewers: + nyquist@chromium.org, zea@chromium.org
+nyquist +zea This looks like some sync-y thing. It would be good to know a bit more history before we remove it.
The CQ bit was checked by smaier@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2068733002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
That's weird. khushalsagar: Any idea why the package name difference client2/client ? I would believe the intent would be to use this class: https://cs.chromium.org/chromium/src/third_party/cacheinvalidation/src/java/c...
nyquist@chromium.org changed reviewers: + khushalsagar@chromium.org
On 2016/06/17 23:13:35, nyquist wrote: > That's weird. khushalsagar: Any idea why the package name difference > client2/client ? > > I would believe the intent would be to use this class: > https://cs.chromium.org/chromium/src/third_party/cacheinvalidation/src/java/c... That looks like a bug. I'm guessing the reason why its client2 there is because in the internal tango repo that's what the package name is and we sed it to client when the library is rolled into Chromium. I don't know what put us in this situation where the manifest was inconsistently updated. The reason why this might not be a problem in practice is because the receiver is only being used to listen to tasks re-scheduled using the android alarm service, and we re-sync if the app is killed or if we transition from background to foreground anyway. I'd go with the change in the first patch here and correct the receiver name. +meek in case he has any comments.
khushalsagar@chromium.org changed reviewers: + meek@chromium.org
The CQ bit was checked by smaier@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2068733002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I've talked to both geels@ and meek@ and they both think this is the right direction. Does anyone want to give it the rubber stamp?
Can you please update the CL description so it matches what you are doing in the patch? Maybe you could clarify it as well by answering these questions: - What is currently happening (or not happening)? As in; what code is not triggered? - Why is that not good? - How does this CL make it better?
Description was changed from ========== This line of the manifest seems broken. Removing it since it shouldn't matter whether it's there or not. com.google.ipc.invalidation.external.client2.contrib.AndroidListener$AlarmReceiver There does seem to be a very similarly named class - com.google.ipc.invalidation.external.client.contrib.AndroidListener Note - client2 vs client. BUG=619937 ========== to ========== This line of the manifest seems broken: com.google.ipc.invalidation.external.client2.contrib.AndroidListener$AlarmReceiver This line is giving Proguard a note, saying that the class doesn't exist. The goal is to remove this note. There does seem to be a very similarly named class - com.google.ipc.invalidation.external.client.contrib.AndroidListener Note - client2 vs client. However, changing to client from client2 does nothing to the final .dex file. Removing the line altogether also does nothing to the .dex file. This change should have no effect on the final .dex file, but will get around a Proguard note. BUG=619937 ==========
On 2016/06/24 18:42:17, nyquist wrote: > Can you please update the CL description so it matches what you are doing in the > patch? > > Maybe you could clarify it as well by answering these questions: > - What is currently happening (or not happening)? As in; what code is not > triggered? > - Why is that not good? > - How does this CL make it better? I have updated it slightly. Some more notes on this: client2 -> client has no effect on the final code. Same can be said with removing the line altogether. This line has been broken for a while, and has not hurt us. My change, when sent out for review, was on patchset 2, which removed the line. On the advice of khushalsagar@, geels@ and meek@, I have instead changed the line to use the proper path. Perhaps they would be better suited to answer why they preferred that route.
The class in question that is referenced from the manifest is an inner class of the core class we use for receiving invalidations for Chrome Sync, so I do believe it will already be in our dex-files from before. It's great that you're fixing the reference though! I believe the path you're fixing is a fallback path when several events happen at the same time, which is why things have mostly been working up until now. Your change will just make it better. Good find! So I would probably change the CL description to something along the lines of: "Fix manifest reference to cache invalidation alarm receiver. Because of a relatively complicated process for rolling in invalidation code to chromium, a mistake earlier happened, where the package name of some classes were changed, but the android manifest for Chrome was not updated. The code in question is relates to a path that is not required for basic usage of invalidation, but fixing the reference makes the invalidation code work better. This was discovered through a proguard note about a missing class, so by updating the reference, the proguard note also disappears."
Description was changed from ========== This line of the manifest seems broken: com.google.ipc.invalidation.external.client2.contrib.AndroidListener$AlarmReceiver This line is giving Proguard a note, saying that the class doesn't exist. The goal is to remove this note. There does seem to be a very similarly named class - com.google.ipc.invalidation.external.client.contrib.AndroidListener Note - client2 vs client. However, changing to client from client2 does nothing to the final .dex file. Removing the line altogether also does nothing to the .dex file. This change should have no effect on the final .dex file, but will get around a Proguard note. BUG=619937 ========== to ========== A mistake happened when rolling in invalidation code to chromium, where the package name of some classes were changed, but the android manifest for Chrome was not updated. The code in question is relates to a path that is not required for basic usage of invalidation, but fixing the reference makes the invalidation code less prone to failure. This was discovered through a Proguard note about a missing class, so by updating the reference, the note also disappears. BUG=619937 ==========
On 2016/06/24 19:59:44, nyquist wrote: > The class in question that is referenced from the manifest is an inner class of > the core class we use for receiving invalidations for Chrome Sync, so I do > believe it will already be in our dex-files from before. > > It's great that you're fixing the reference though! I believe the path you're > fixing is a fallback path when several events happen at the same time, which is > why things have mostly been working up until now. Your change will just make it > better. Good find! > > So I would probably change the CL description to something along the lines of: > "Fix manifest reference to cache invalidation alarm receiver. > > Because of a relatively complicated process for rolling in invalidation > code to chromium, a mistake earlier happened, where the package name of some > classes > were changed, but the android manifest for Chrome was not updated. > > The code in question is relates to a path that is not required for basic usage > of invalidation, but fixing the reference makes the invalidation code work > better. > > This was discovered through a proguard note about a missing class, so by > updating > the reference, the proguard note also disappears." I've updated the CL description. Let me know if that looks good.
lgtm
Description was changed from ========== A mistake happened when rolling in invalidation code to chromium, where the package name of some classes were changed, but the android manifest for Chrome was not updated. The code in question is relates to a path that is not required for basic usage of invalidation, but fixing the reference makes the invalidation code less prone to failure. This was discovered through a Proguard note about a missing class, so by updating the reference, the note also disappears. BUG=619937 ========== to ========== Fix manifest reference to cache invalidation alarm receiver. A mistake happened when rolling in invalidation code to chromium, where the package name of some classes were changed, but the android manifest for Chrome was not updated. The code in question is relates to a path that is not required for basic usage of invalidation, but fixing the reference makes the invalidation code less prone to failure. This was discovered through a Proguard note about a missing class, so by updating the reference, the note also disappears. BUG=619937 ==========
The CQ bit was checked by smaier@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fix manifest reference to cache invalidation alarm receiver. A mistake happened when rolling in invalidation code to chromium, where the package name of some classes were changed, but the android manifest for Chrome was not updated. The code in question is relates to a path that is not required for basic usage of invalidation, but fixing the reference makes the invalidation code less prone to failure. This was discovered through a Proguard note about a missing class, so by updating the reference, the note also disappears. BUG=619937 ========== to ========== Fix manifest reference to cache invalidation alarm receiver. A mistake happened when rolling in invalidation code to chromium, where the package name of some classes were changed, but the android manifest for Chrome was not updated. The code in question is relates to a path that is not required for basic usage of invalidation, but fixing the reference makes the invalidation code less prone to failure. This was discovered through a Proguard note about a missing class, so by updating the reference, the note also disappears. BUG=619937 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Fix manifest reference to cache invalidation alarm receiver. A mistake happened when rolling in invalidation code to chromium, where the package name of some classes were changed, but the android manifest for Chrome was not updated. The code in question is relates to a path that is not required for basic usage of invalidation, but fixing the reference makes the invalidation code less prone to failure. This was discovered through a Proguard note about a missing class, so by updating the reference, the note also disappears. BUG=619937 ========== to ========== Fix manifest reference to cache invalidation alarm receiver. A mistake happened when rolling in invalidation code to chromium, where the package name of some classes were changed, but the android manifest for Chrome was not updated. The code in question is relates to a path that is not required for basic usage of invalidation, but fixing the reference makes the invalidation code less prone to failure. This was discovered through a Proguard note about a missing class, so by updating the reference, the note also disappears. BUG=619937 Committed: https://crrev.com/0a191afe97691594f1cee9e33cdeb5b7a2f51e87 Cr-Commit-Position: refs/heads/master@{#404182} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/0a191afe97691594f1cee9e33cdeb5b7a2f51e87 Cr-Commit-Position: refs/heads/master@{#404182} |
