Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2186053002:
Add context menu to snippets. (Closed)
DescriptionAdd context menu to snippets.
BUG=631939
Committed: https://crrev.com/0b896a0f9a0922a20a9968b9562ca7bce562fd8f
Cr-Commit-Position: refs/heads/master@{#408609}
Patch Set 1 #
Total comments: 27
Patch Set 2 : Move dismiss to CardViewHolder and added alpha animation. #Patch Set 3 : Add comments. #
Total comments: 4
Patch Set 4 : Simplify. #Patch Set 5 : Merge. #
Messages
Total messages: 27 (14 generated)
peconn@chromium.org changed reviewers: + bauerb@chromium.org, dgn@chromium.org
PTAL. https://codereview.chromium.org/2186053002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2186053002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:294: public void onCreateContextMenu(ContextMenu menu, OnMenuItemClickListener listener) { This method and the one below it are only used for MostVisitedItem. I'm half tempted to remove these methods from here and put the logic (which items to add, what to do on click) in MostVisitedItem and just leave the stubs (isOpenInNewWindowEnabled, etc) in the NewTabPageManager, but at that point it would make sense getting rid of MostVisitedItemManager entirely. https://codereview.chromium.org/2186053002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:296: if (isOpenInNewWindowEnabled()) { Did you know we had this? It looks like its something to do with multi window support on N. Exciting! https://codereview.chromium.org/2186053002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:347: public void openUrlInNewWindow(String url) { Do you think it's worth putting assert !mIsDestroyed in these as in openUrl? https://codereview.chromium.org/2186053002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2186053002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:160: protected void onCreateContextMenu(ContextMenu menu) { TODO(peconn): Add a NewTabPageManager.isDestroyed() check here. https://codereview.chromium.org/2186053002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:217: .dismissItem(SnippetArticleViewHolder.this); I'm not very happy with this line - it's the sole reason we hold onto the RecyclerView, but I'm not sure how else to do it.
https://codereview.chromium.org/2186053002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2186053002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:347: public void openUrlInNewWindow(String url) { On 2016/07/27 16:42:18, PEConn1 wrote: > Do you think it's worth putting assert !mIsDestroyed in these as in openUrl? Not necessarily – see my other comment below. https://codereview.chromium.org/2186053002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/2186053002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:158: /** Opens an url in the current tab. */ Nit: "a URL" is correct, because the "U" is pronounced /ˈjuː/ :) It should also be URL to make it clear that it's an acronym. https://codereview.chromium.org/2186053002/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/2186053002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java:194: protected void onCreateContextMenu(ContextMenu menu) {} Just createContextMenu()? https://codereview.chromium.org/2186053002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2186053002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:157: * Create a context menu akin to the one shown for MostVisitedItems. If you override this method from the base class, it might make more sense to make this comment an implementation comment. https://codereview.chromium.org/2186053002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:160: protected void onCreateContextMenu(ContextMenu menu) { On 2016/07/27 16:42:18, PEConn1 wrote: > TODO(peconn): Add a NewTabPageManager.isDestroyed() check here. Eh. I think those are really only needed for when we might get unexpected callbacks, like from native code. These views are owned by the NewTabPage, so they shouldn't be active anymore when the NTP is destroyed. https://codereview.chromium.org/2186053002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:217: .dismissItem(SnippetArticleViewHolder.this); On 2016/07/27 16:42:19, PEConn1 wrote: > I'm not very happy with this line - it's the sole reason we hold onto the > RecyclerView, but I'm not sure how else to do it. Hm, we have to call out to _something_. If we wanted, we could hide this behind an interface and inject it on construction, but that feels like overkill to me. Or alternatively make it a method on NewTabPageManager, although that's also starting to get pretty big… Maybe we need a different interface that's specific to card actions? Also, what we should maybe do is move this animation code to the RecyclerView or so?
https://codereview.chromium.org/2186053002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2186053002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:354: public void openUrlInNewTab(String url) { how about having openUrlInNewTab(String url, bool incognito) ? https://codereview.chromium.org/2186053002/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/2186053002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java:194: protected void onCreateContextMenu(ContextMenu menu) {} On 2016/07/27 18:29:53, Bernhard Bauer wrote: > Just createContextMenu()? The current form is consistent with onCardTapped() above. https://codereview.chromium.org/2186053002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2186053002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:208: animator.addListener(new Animator.AnimatorListener() { Use AnimatorListenerAdapter instead? https://codereview.chromium.org/2186053002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:217: .dismissItem(SnippetArticleViewHolder.this); On 2016/07/27 18:29:53, Bernhard Bauer wrote: > On 2016/07/27 16:42:19, PEConn1 wrote: > > I'm not very happy with this line - it's the sole reason we hold onto the > > RecyclerView, but I'm not sure how else to do it. > > Hm, we have to call out to _something_. If we wanted, we could hide this behind > an interface and inject it on construction, but that feels like overkill to me. > Or alternatively make it a method on NewTabPageManager, although that's also > starting to get pretty big… Maybe we need a different interface that's specific > to card actions? > > Also, what we should maybe do is move this animation code to the RecyclerView or > so? CardViewHolder has a reference to RecyclerView. How about implementing it one level lower? It also makes sense in the parent class.
https://codereview.chromium.org/2186053002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2186053002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:217: .dismissItem(SnippetArticleViewHolder.this); On 2016/07/27 18:58:30, dgn wrote: > On 2016/07/27 18:29:53, Bernhard Bauer wrote: > > On 2016/07/27 16:42:19, PEConn1 wrote: > > > I'm not very happy with this line - it's the sole reason we hold onto the > > > RecyclerView, but I'm not sure how else to do it. > > > > Hm, we have to call out to _something_. If we wanted, we could hide this > behind > > an interface and inject it on construction, but that feels like overkill to > me. > > Or alternatively make it a method on NewTabPageManager, although that's also > > starting to get pretty big… Maybe we need a different interface that's > specific > > to card actions? > > > > Also, what we should maybe do is move this animation code to the RecyclerView > or > > so? > > CardViewHolder has a reference to RecyclerView. How about implementing it one > level lower? It also makes sense in the parent class. Will all CardViewHolder's have the same behaviour? I would have thought we'd want to implement different context menus for each one. Or did you mean just implement dismiss at the CardViewHolder level? Are all CardViewHolders meant to be dismissed (what about the StatusListItem?)? Currently NewTabPageAdapter.dismiss() casts to a SnippetArticleViewHolder. Maybe I've misunderstood what you're suggesting.
https://codereview.chromium.org/2186053002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2186053002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:217: .dismissItem(SnippetArticleViewHolder.this); On 2016/07/27 19:14:39, PEConn1 wrote: > On 2016/07/27 18:58:30, dgn wrote: > > On 2016/07/27 18:29:53, Bernhard Bauer wrote: > > > On 2016/07/27 16:42:19, PEConn1 wrote: > > > > I'm not very happy with this line - it's the sole reason we hold onto the > > > > RecyclerView, but I'm not sure how else to do it. > > > > > > Hm, we have to call out to _something_. If we wanted, we could hide this > > behind > > > an interface and inject it on construction, but that feels like overkill to > > me. > > > Or alternatively make it a method on NewTabPageManager, although that's also > > > starting to get pretty big… Maybe we need a different interface that's > > specific > > > to card actions? > > > > > > Also, what we should maybe do is move this animation code to the > RecyclerView > > or > > > so? > > > > CardViewHolder has a reference to RecyclerView. How about implementing it one > > level lower? It also makes sense in the parent class. > > Will all CardViewHolder's have the same behaviour? I would have thought we'd > want to implement different context menus for each one. > > Or did you mean just implement dismiss at the CardViewHolder level? Are all > CardViewHolders meant to be dismissed (what about the StatusListItem?)? > Currently NewTabPageAdapter.dismiss() casts to a SnippetArticleViewHolder. > > Maybe I've misunderstood what you're suggesting. I mean just dismiss(). CardViewHolders already have updateViewStateForDismiss(), inherit isDismissable() from the parent, etc. Even if they don't all have the same behaviour, dismiss() can be implemented at that level, possibly check that isDismissible() returns true when called.
https://codereview.chromium.org/2186053002/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/2186053002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java:194: protected void onCreateContextMenu(ContextMenu menu) {} On 2016/07/27 18:58:30, dgn wrote: > On 2016/07/27 18:29:53, Bernhard Bauer wrote: > > Just createContextMenu()? > > The current form is consistent with onCardTapped() above. Yes, but that reads like an event (a card was tapped). Here it's phrased like an imperative (create the context menu!), so the "on" prefix makes less sense. https://codereview.chromium.org/2186053002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2186053002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:217: .dismissItem(SnippetArticleViewHolder.this); On 2016/07/27 19:14:39, PEConn1 wrote: > On 2016/07/27 18:58:30, dgn wrote: > > On 2016/07/27 18:29:53, Bernhard Bauer wrote: > > > On 2016/07/27 16:42:19, PEConn1 wrote: > > > > I'm not very happy with this line - it's the sole reason we hold onto the > > > > RecyclerView, but I'm not sure how else to do it. > > > > > > Hm, we have to call out to _something_. If we wanted, we could hide this > > behind > > > an interface and inject it on construction, but that feels like overkill to > > me. > > > Or alternatively make it a method on NewTabPageManager, although that's also > > > starting to get pretty big… Maybe we need a different interface that's > > specific > > > to card actions? > > > > > > Also, what we should maybe do is move this animation code to the > RecyclerView > > or > > > so? > > > > CardViewHolder has a reference to RecyclerView. How about implementing it one > > level lower? It also makes sense in the parent class. > > Will all CardViewHolder's have the same behaviour? I would have thought we'd > want to implement different context menus for each one. > > Or did you mean just implement dismiss at the CardViewHolder level? Are all > CardViewHolders meant to be dismissed (what about the StatusListItem?)? > Currently NewTabPageAdapter.dismiss() casts to a SnippetArticleViewHolder. I think it makes sense to allow any UI element that is a card to be dismissed (at least in code -- whether we choose to allow dismissing e.g. status cards is a decision at a different level), because that goes with the physical metaphor of a card. Dismissing an article specifically would then have the additional side effect of notifying the backend. We could do that with an observer though, or an empty base method that can be overridden. > Maybe I've misunderstood what you're suggesting.
https://codereview.chromium.org/2186053002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2186053002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:347: public void openUrlInNewWindow(String url) { On 2016/07/27 18:29:53, Bernhard Bauer wrote: > On 2016/07/27 16:42:18, PEConn1 wrote: > > Do you think it's worth putting assert !mIsDestroyed in these as in openUrl? > > Not necessarily – see my other comment below. Acknowledged. https://codereview.chromium.org/2186053002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:354: public void openUrlInNewTab(String url) { On 2016/07/27 18:58:30, dgn wrote: > how about having openUrlInNewTab(String url, bool incognito) ? Done. https://codereview.chromium.org/2186053002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/2186053002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:158: /** Opens an url in the current tab. */ On 2016/07/27 18:29:53, Bernhard Bauer wrote: > Nit: "a URL" is correct, because the "U" is pronounced /ˈjuː/ :) It should also > be URL to make it clear that it's an acronym. Done - though I'm half tempted to document that! https://codereview.chromium.org/2186053002/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/2186053002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java:194: protected void onCreateContextMenu(ContextMenu menu) {} On 2016/07/28 09:12:09, Bernhard Bauer wrote: > On 2016/07/27 18:58:30, dgn wrote: > > On 2016/07/27 18:29:53, Bernhard Bauer wrote: > > > Just createContextMenu()? > > > > The current form is consistent with onCardTapped() above. > > Yes, but that reads like an event (a card was tapped). Here it's phrased like an > imperative (create the context menu!), so the "on" prefix makes less sense. Done. I think the Android convention here is a bit misleading for the same reason (they have 'onCreateContextMenu' which should really be something like 'onCreateContextMenuRequested' or 'createContextMenu') https://codereview.chromium.org/2186053002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2186053002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:157: * Create a context menu akin to the one shown for MostVisitedItems. On 2016/07/27 18:29:53, Bernhard Bauer wrote: > If you override this method from the base class, it might make more sense to > make this comment an implementation comment. Done. https://codereview.chromium.org/2186053002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:160: protected void onCreateContextMenu(ContextMenu menu) { On 2016/07/27 18:29:53, Bernhard Bauer wrote: > On 2016/07/27 16:42:18, PEConn1 wrote: > > TODO(peconn): Add a NewTabPageManager.isDestroyed() check here. > > Eh. I think those are really only needed for when we might get unexpected > callbacks, like from native code. These views are owned by the NewTabPage, so > they shouldn't be active anymore when the NTP is destroyed. Done. https://codereview.chromium.org/2186053002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:208: animator.addListener(new Animator.AnimatorListener() { On 2016/07/27 18:58:30, dgn wrote: > Use AnimatorListenerAdapter instead? Done. https://codereview.chromium.org/2186053002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:217: .dismissItem(SnippetArticleViewHolder.this); On 2016/07/28 09:12:09, Bernhard Bauer wrote: > On 2016/07/27 19:14:39, PEConn1 wrote: > > On 2016/07/27 18:58:30, dgn wrote: > > > On 2016/07/27 18:29:53, Bernhard Bauer wrote: > > > > On 2016/07/27 16:42:19, PEConn1 wrote: > > > > > I'm not very happy with this line - it's the sole reason we hold onto > the > > > > > RecyclerView, but I'm not sure how else to do it. > > > > > > > > Hm, we have to call out to _something_. If we wanted, we could hide this > > > behind > > > > an interface and inject it on construction, but that feels like overkill > to > > > me. > > > > Or alternatively make it a method on NewTabPageManager, although that's > also > > > > starting to get pretty big… Maybe we need a different interface that's > > > specific > > > > to card actions? > > > > > > > > Also, what we should maybe do is move this animation code to the > > RecyclerView > > > or > > > > so? > > > > > > CardViewHolder has a reference to RecyclerView. How about implementing it > one > > > level lower? It also makes sense in the parent class. > > > > Will all CardViewHolder's have the same behaviour? I would have thought we'd > > want to implement different context menus for each one. > > > > Or did you mean just implement dismiss at the CardViewHolder level? Are all > > CardViewHolders meant to be dismissed (what about the StatusListItem?)? > > Currently NewTabPageAdapter.dismiss() casts to a SnippetArticleViewHolder. > > I think it makes sense to allow any UI element that is a card to be dismissed > (at least in code -- whether we choose to allow dismissing e.g. status cards is > a decision at a different level), because that goes with the physical metaphor > of a card. Dismissing an article specifically would then have the additional > side effect of notifying the backend. We could do that with an observer though, > or an empty base method that can be overridden. > > > Maybe I've misunderstood what you're suggesting. > Done.
https://codereview.chromium.org/2186053002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java (right): https://codereview.chromium.org/2186053002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java:255: if (!isDismissable()) return; Maybe assert instead? https://codereview.chromium.org/2186053002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2186053002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:197: adapter.dismissItem(this); Actually, we could just directly call this on the adapter... The adapter knows when a dismissed item was an article and when it wasn't.
https://codereview.chromium.org/2186053002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java (right): https://codereview.chromium.org/2186053002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java:255: if (!isDismissable()) return; On 2016/07/28 15:04:40, Bernhard Bauer wrote: > Maybe assert instead? Done. https://codereview.chromium.org/2186053002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2186053002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:197: adapter.dismissItem(this); On 2016/07/28 15:04:40, Bernhard Bauer wrote: > Actually, we could just directly call this on the adapter... The adapter knows > when a dismissed item was an article and when it wasn't. Done.
The CQ bit was checked by peconn@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: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
lgtm
The CQ bit was checked by peconn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/2186053002/#ps80001 (title: "Merge.")
The CQ bit was unchecked by peconn@chromium.org
The CQ bit was checked by peconn@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 peconn@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.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Add context menu to snippets. BUG=631939 ========== to ========== Add context menu to snippets. BUG=631939 Committed: https://crrev.com/0b896a0f9a0922a20a9968b9562ca7bce562fd8f Cr-Commit-Position: refs/heads/master@{#408609} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/0b896a0f9a0922a20a9968b9562ca7bce562fd8f Cr-Commit-Position: refs/heads/master@{#408609} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
