Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2588683002:
Disable Share extension item receiver if there is no app group. (Closed)
DescriptionDisable Share extension item receiver if there is no app group.
EGTests don't have the application group entitlement.
Disable the Receiver in that case.
BUG=675412
Committed: https://crrev.com/2d46b12dc7095b8aed4bc2c496b8dd9be3e78e0f
Cr-Commit-Position: refs/heads/master@{#439754}
Patch Set 1 #
Total comments: 2
Messages
Total messages: 18 (6 generated)
olivierrobin@chromium.org changed reviewers: + rohitrao@chromium.org
https://codereview.chromium.org/2588683002/diff/1/ios/chrome/browser/share_ex... File ios/chrome/browser/share_extension/share_extension_item_receiver.mm (left): https://codereview.chromium.org/2588683002/diff/1/ios/chrome/browser/share_ex... ios/chrome/browser/share_extension/share_extension_item_receiver.mm:101: #if TARGET_IPHONE_SIMULATOR We're running egtests on devices as well, that's where I first noticed the issue. I'm not sure I've ever actually seen this crash on simulator. Will this change cause any tests to fail, since we're effectively turning off functionality? Should we add the entitlement to egtests instead?
> We're running egtests on devices as well, that's where I first noticed the > issue. I'm not sure I've ever actually seen this crash on simulator. Ignore this comment, I read the diff backwards =) My main question still applies though -- should we try to add the proper entitlements to EG tests? Is that even possible?
On 2016/12/19 09:42:01, rohitrao wrote: > > We're running egtests on devices as well, that's where I first noticed the > > issue. I'm not sure I've ever actually seen this crash on simulator. > > Ignore this comment, I read the diff backwards =) > > > My main question still applies though -- should we try to add the proper > entitlements to EG tests? Is that even possible? Yes, we should try to have app_group in egtests. This is a temporary fix and I will let the bug 675412 open. There should be no test failing as there is the receiver for other processes.
On 2016/12/19 10:18:48, Olivier Robin wrote: > On 2016/12/19 09:42:01, rohitrao wrote: > > > We're running egtests on devices as well, that's where I first noticed the > > > issue. I'm not sure I've ever actually seen this crash on simulator. > > > > Ignore this comment, I read the diff backwards =) > > > > > > My main question still applies though -- should we try to add the proper > > entitlements to EG tests? Is that even possible? > > > Yes, we should try to have app_group in egtests. > This is a temporary fix and I will let the bug 675412 open. > There should be no test failing as there is the receiver for other processes. Last sentence in correct English There should be no test failing as there is no test for receiving items from other processes.
rohitrao@chromium.org changed reviewers: + baxley@chromium.org, sdefresne@chromium.org
LGTM +baxley and sdefresne FYI, we should try to get the proper entitlements in place for EG long-term.
On 2016/12/20 00:53:45, rohitrao wrote: > LGTM > > +baxley and sdefresne FYI, we should try to get the proper entitlements in place > for EG long-term. Can you file a bug? There is multiple way to resolve this with pros/cons, and I'd like to discuss them there. Add me, justincohen and baxley in cc to the but.
lgtm https://codereview.chromium.org/2588683002/diff/1/ios/chrome/browser/share_ex... File ios/chrome/browser/share_extension/share_extension_item_receiver.mm (left): https://codereview.chromium.org/2588683002/diff/1/ios/chrome/browser/share_ex... ios/chrome/browser/share_extension/share_extension_item_receiver.mm:101: #if TARGET_IPHONE_SIMULATOR On 2016/12/19 09:40:33, rohitrao wrote: > We're running egtests on devices as well, that's where I first noticed the > issue. I'm not sure I've ever actually seen this crash on simulator. > > Will this change cause any tests to fail, since we're effectively turning off > functionality? > > Should we add the entitlement to egtests instead? I think we should try to get the entitlements for egtests, but it needs some work. For the moment, I think it is fine just turning this off.
On 2016/12/20 01:01:57, sdefresne wrote: > On 2016/12/20 00:53:45, rohitrao wrote: > > LGTM > > > > +baxley and sdefresne FYI, we should try to get the proper entitlements in > place > > for EG long-term. > > Can you file a bug? There is multiple way to resolve this with pros/cons, and > I'd like to discuss them there. > Add me, justincohen and baxley in cc to the but. I filed: https://bugs.chromium.org/p/chromium/issues/detail?id=675852 If someone already filed one, please close it. Thanks!
The CQ bit was checked by olivierrobin@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": 1, "attempt_start_ts": 1482221961651180, "parent_rev":
"96229ab95e858c160c89ceea798bf3491e7ff902", "commit_rev":
"ef5dca898d371752617a81b51e66636ed3525adc"}
Message was sent while issue was closed.
Description was changed from ========== Disable Share extension item receiver if there is no app group. EGTests don't have the application group entitlement. Disable the Receiver in that case. BUG=675412 ========== to ========== Disable Share extension item receiver if there is no app group. EGTests don't have the application group entitlement. Disable the Receiver in that case. BUG=675412 Review-Url: https://codereview.chromium.org/2588683002 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Disable Share extension item receiver if there is no app group. EGTests don't have the application group entitlement. Disable the Receiver in that case. BUG=675412 Review-Url: https://codereview.chromium.org/2588683002 ========== to ========== Disable Share extension item receiver if there is no app group. EGTests don't have the application group entitlement. Disable the Receiver in that case. BUG=675412 Committed: https://crrev.com/2d46b12dc7095b8aed4bc2c496b8dd9be3e78e0f Cr-Commit-Position: refs/heads/master@{#439754} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/2d46b12dc7095b8aed4bc2c496b8dd9be3e78e0f Cr-Commit-Position: refs/heads/master@{#439754} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
