Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2247763002:
Check NTP Snippet index before dismissing. (Closed)
DescriptionCheck NTP Snippet index before dismissing.
Check the index of a Snippet on the NTP before dismissing it to ensure it is not deleted twice.
BUG=636296
Committed: https://crrev.com/1a9f42cae0856586b9c7ce7099ea5f614b9f04cd
Cr-Commit-Position: refs/heads/master@{#412044}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Fix comment. #Messages
Total messages: 21 (9 generated)
peconn@chromium.org changed reviewers: + bauerb@chromium.org, mvanouwerkerk@chromium.org
This is a quick and simple fix. I considered disabling swiping on an item when the long press menu was summoned, but I could not find a way to be notified when a ContextMenu was dismissed. I also had a look at changing the ContextMenu to a PopupMenu (which does have dismiss listeners), but PopupMenu would anchor to the item it was summoned for.
The CQ bit was checked by mvanouwerkerk@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...
Could you update the description to give some more context (e.g. Android, NTP)? On 2016/08/15 15:23:28, PEConn wrote: > This is a quick and simple fix. > > I considered disabling swiping on an item when the long press menu was summoned, > but I could not find a way to be notified when a ContextMenu was dismissed. Would overriding https://developer.android.com/reference/android/app/Activity.html#onContextMe... work? https://codereview.chromium.org/2247763002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java (right): https://codereview.chromium.org/2247763002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java:261: // In case the user pressed dismiss after swiping to dismiss the Nit: the what?
lgtm with nit to fix the crash I think the prettier fix would be lower priority, this is really the kind of edge case that mostly just our qa team runs into. https://codereview.chromium.org/2247763002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java (right): https://codereview.chromium.org/2247763002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java:261: // In case the user pressed dismiss after swiping to dismiss the nit: unfinished sentence?
Description was changed from ========== Check item index before deleting. BUG=636296 ========== to ========== Check NTP Snippet index before dismissing. Check the index of a Snippet on the NTP before dismissing it to ensure it is not deleted twice. BUG=636296 ==========
On 2016/08/15 15:38:15, Bernhard Bauer wrote: > Could you update the description to give some more context (e.g. Android, NTP)? > > On 2016/08/15 15:23:28, PEConn wrote: > > This is a quick and simple fix. > > > > I considered disabling swiping on an item when the long press menu was > summoned, > > but I could not find a way to be notified when a ContextMenu was dismissed. > > Would overriding > https://developer.android.com/reference/android/app/Activity.html#onContextMe... > work? > > https://codereview.chromium.org/2247763002/diff/1/chrome/android/java/src/org... > File > chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java > (right): > > https://codereview.chromium.org/2247763002/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java:261: > // In case the user pressed dismiss after swiping to dismiss the > Nit: the what? Activity#onContextMenuClosed could work, but it seems a bit like cracking a walnut with a sledgehammer - adding a listener at the Activity level to catch dismissing a menu this far down the hierarchy.
The CQ bit was unchecked by peconn@chromium.org
https://codereview.chromium.org/2247763002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java (right): https://codereview.chromium.org/2247763002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java:261: // In case the user pressed dismiss after swiping to dismiss the On 2016/08/15 15:38:15, Bernhard Bauer wrote: > Nit: the what? Done. https://codereview.chromium.org/2247763002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java:261: // In case the user pressed dismiss after swiping to dismiss the On 2016/08/15 15:39:29, Michael van Ouwerkerk wrote: > nit: unfinished sentence? Done.
On 2016/08/15 15:42:37, PEConn wrote: > Activity#onContextMenuClosed could work, but it seems a bit like cracking a > walnut with a sledgehammer - adding a listener at the Activity level to catch > dismissing a menu this far down the hierarchy. Eh, we'd need to add some plumbing, but if listening to context menu dismissal is what we want to do, it would be the way to go. But this is fine with me as well, so LGTM.
On 2016/08/15 16:45:39, Bernhard Bauer wrote: > On 2016/08/15 15:42:37, PEConn wrote: > > Activity#onContextMenuClosed could work, but it seems a bit like cracking a > > walnut with a sledgehammer - adding a listener at the Activity level to catch > > dismissing a menu this far down the hierarchy. > > Eh, we'd need to add some plumbing, but if listening to context menu dismissal > is what we want to do, it would be the way to go. But this is fine with me as > well, so LGTM. Additionally, wouldn't we need to add the functionality to multiple activities? ChromeTabbedActivity, ChromeTabbedActivity2 (?), CustomTabActivity, DocumentActivity?
On Mon, Aug 15, 2016, 09:51 <peconn@chromium.org> wrote: > On 2016/08/15 16:45:39, Bernhard Bauer wrote: > > On 2016/08/15 15:42:37, PEConn wrote: > > > Activity#onContextMenuClosed could work, but it seems a bit like > cracking a > > > walnut with a sledgehammer - adding a listener at the Activity level to > catch > > > dismissing a menu this far down the hierarchy. > > > > Eh, we'd need to add some plumbing, but if listening to context menu > dismissal > > is what we want to do, it would be the way to go. But this is fine with > me as > > well, so LGTM. > > Additionally, wouldn't we need to add the functionality to multiple > activities? > ChromeTabbedActivity, ChromeTabbedActivity2 (?), CustomTabActivity, > DocumentActivity? > Or just the base class ChromeActivity. https://codereview.chromium.org/2247763002/ > > -- > You received this message because you are subscribed to the Google Groups > "New Tab Page development" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to ntp-dev+unsubscribe@chromium.org. > To post to this group, send email to ntp-dev@chromium.org. > To view this discussion on the web visit > https://groups.google.com/a/chromium.org/d/msgid/ntp-dev/bcaec5216193cade1f05... > <https://groups.google.com/a/chromium.org/d/msgid/ntp-dev/bcaec5216193cade1f05...> > . > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/08/15 16:53:59, Bernhard Bauer wrote: > On Mon, Aug 15, 2016, 09:51 <mailto:peconn@chromium.org> wrote: > > > On 2016/08/15 16:45:39, Bernhard Bauer wrote: > > > On 2016/08/15 15:42:37, PEConn wrote: > > > > Activity#onContextMenuClosed could work, but it seems a bit like > > cracking a > > > > walnut with a sledgehammer - adding a listener at the Activity level to > > catch > > > > dismissing a menu this far down the hierarchy. > > > > > > Eh, we'd need to add some plumbing, but if listening to context menu > > dismissal > > > is what we want to do, it would be the way to go. But this is fine with > > me as > > > well, so LGTM. > > > > Additionally, wouldn't we need to add the functionality to multiple > > activities? > > ChromeTabbedActivity, ChromeTabbedActivity2 (?), CustomTabActivity, > > DocumentActivity? > > > > Or just the base class ChromeActivity. > > https://codereview.chromium.org/2247763002/ > > > > -- > > You received this message because you are subscribed to the Google Groups > > "New Tab Page development" group. > > To unsubscribe from this group and stop receiving emails from it, send an > > email to mailto:ntp-dev+unsubscribe@chromium.org. > > To post to this group, send email to mailto:ntp-dev@chromium.org. > > To view this discussion on the web visit > > > https://groups.google.com/a/chromium.org/d/msgid/ntp-dev/bcaec5216193cade1f05... > > > <https://groups.google.com/a/chromium.org/d/msgid/ntp-dev/bcaec5216193cade1f05...> > > . > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Ah OK, it looks like that class does currently do something similar: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...
The CQ bit was checked by peconn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mvanouwerkerk@chromium.org Link to the patchset: https://codereview.chromium.org/2247763002/#ps20001 (title: "Fix comment.")
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 ========== Check NTP Snippet index before dismissing. Check the index of a Snippet on the NTP before dismissing it to ensure it is not deleted twice. BUG=636296 ========== to ========== Check NTP Snippet index before dismissing. Check the index of a Snippet on the NTP before dismissing it to ensure it is not deleted twice. BUG=636296 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Check NTP Snippet index before dismissing. Check the index of a Snippet on the NTP before dismissing it to ensure it is not deleted twice. BUG=636296 ========== to ========== Check NTP Snippet index before dismissing. Check the index of a Snippet on the NTP before dismissing it to ensure it is not deleted twice. BUG=636296 Committed: https://crrev.com/1a9f42cae0856586b9c7ce7099ea5f614b9f04cd Cr-Commit-Position: refs/heads/master@{#412044} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/1a9f42cae0856586b9c7ce7099ea5f614b9f04cd Cr-Commit-Position: refs/heads/master@{#412044} |
