Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1877143002:
Convert ToolbarActionsBarBubbleViews to BubbleDialogDelegate (Closed)
|
Created:
4 years, 8 months ago by Evan Stade Modified:
4 years, 8 months ago CC:
chromium-reviews, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionConvert ToolbarActionsBarBubbleViews to BubbleDialogDelegate
While we're at it, fix a bug where the dismiss button was treated
identically to the confirm button.
BUG=585312
Committed: https://crrev.com/dc0d234f7c87d29e9d070249a5dd601141faff5b
Cr-Commit-Position: refs/heads/master@{#387219}
Patch Set 1 #
Total comments: 4
Patch Set 2 : rebase #Patch Set 3 : dismiss reason #Patch Set 4 : fix test #
Total comments: 6
Patch Set 5 : rebase #
Total comments: 1
Patch Set 6 : fix layout #
Total comments: 6
Patch Set 7 : . #Messages
Total messages: 40 (13 generated)
estade@chromium.org changed reviewers: + msw@chromium.org, rdevlin.cronin@chromium.org
before/after: https://screenshot.googleplex.com/kGsZ6VZYVf6.png
lgtm
The CQ bit was checked by estade@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/1877143002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1877143002/1
On 2016/04/12 00:03:27, Evan Stade wrote: > before/after: https://screenshot.googleplex.com/kGsZ6VZYVf6.png I think this is going to conflict with https://codereview.chromium.org/1858773006/ and https://codereview.chromium.org/1881773002/, which convert ExtensionMessageBubbles to use the ToolbarActionsBarBubble. Can we hold off on this and look again once that code lands?
On 2016/04/12 00:44:41, Devlin wrote: > On 2016/04/12 00:03:27, Evan Stade wrote: > > before/after: https://screenshot.googleplex.com/kGsZ6VZYVf6.png > > I think this is going to conflict with > https://codereview.chromium.org/1858773006/ and > https://codereview.chromium.org/1881773002/, which convert > ExtensionMessageBubbles to use the ToolbarActionsBarBubble. Can we hold off on > this and look again once that code lands? (I'd be happy to take over the conversion if it still makes sense to save you the effort, if that's easier.)
On 2016/04/12 00:46:23, Devlin wrote: > On 2016/04/12 00:44:41, Devlin wrote: > > On 2016/04/12 00:03:27, Evan Stade wrote: > > > before/after: https://screenshot.googleplex.com/kGsZ6VZYVf6.png > > > > I think this is going to conflict with > > https://codereview.chromium.org/1858773006/ and > > https://codereview.chromium.org/1881773002/, which convert > > ExtensionMessageBubbles to use the ToolbarActionsBarBubble. Can we hold off > on > > this and look again once that code lands? > > (I'd be happy to take over the conversion if it still makes sense to save you > the effort, if that's easier.) yes, both these bubbles should be converted. I looked at them and thought that converting them first would make it easier to merge them. I don't think this CL actually conflicts (beyond a couple lines) with the one you already have LG'd and doesn't conflict that much with #2 either. i.e., this one can land, and the only real difference is that your learn more link will be an ExtraView.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1877143002/diff/1/chrome/browser/ui/views/too... File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc (right): https://codereview.chromium.org/1877143002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:34: // but this reason isn't used for anything currently. This will be used after https://codereview.chromium.org/1881773002/. I'd like to see if we can get the close reason to be correct in this patch so that we don't lose functionality that will be needed for combining with ExtensionMessageBubble. https://codereview.chromium.org/1877143002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:54: return button == ui::DIALOG_BUTTON_OK ? delegate_->GetActionButtonText() optional nit: I'd slightly prefer a switch here so that we can NOTREACHED on ui::DIALOG_BUTTON_NONE and DCHECK(!delegate_->GetDismissButtonText().empty()) on ui::DIALOG_BUTTON_CANCEL, and if we ever add something like a ui::DIALOG_BUTTON_LEARN_MORE or lump close buttons in there or anything, we get a compile error.
The CQ bit was checked by estade@chromium.org to run a CQ dry run
https://codereview.chromium.org/1877143002/diff/1/chrome/browser/ui/views/too... File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc (right): https://codereview.chromium.org/1877143002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:34: // but this reason isn't used for anything currently. On 2016/04/12 23:01:05, Devlin wrote: > This will be used after https://codereview.chromium.org/1881773002/. I'd like > to see if we can get the close reason to be correct in this patch so that we > don't lose functionality that will be needed for combining with > ExtensionMessageBubble. Done. https://codereview.chromium.org/1877143002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:54: return button == ui::DIALOG_BUTTON_OK ? delegate_->GetActionButtonText() On 2016/04/12 23:01:05, Devlin wrote: > optional nit: I'd slightly prefer a switch here so that we can NOTREACHED on > ui::DIALOG_BUTTON_NONE and DCHECK(!delegate_->GetDismissButtonText().empty()) on > ui::DIALOG_BUTTON_CANCEL, and if we ever add something like a > ui::DIALOG_BUTTON_LEARN_MORE or lump close buttons in there or anything, we get > a compile error. I see what you're saying, but this pattern is pretty standard across dialogs.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1877143002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1877143002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm with nit + question, assuming the compile fix doesn't change much. https://codereview.chromium.org/1877143002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc (right): https://codereview.chromium.org/1877143002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:35: bool ToolbarActionsBarBubbleViews::Cancel() { Is this called when the user hits escape? https://codereview.chromium.org/1877143002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc (left): https://codereview.chromium.org/1877143002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc:55: EXPECT_EQ(HeadingString(), bubble->heading_label()->text()); thinking about this, I'd prefer to keep the text checks here. I'm sure BubbleDialogDelegateView probably has them, too, but this also serves to test that the delegate passes up the right string for each button. And, really, it's just a super cheap check in a unittest, so I'd say that it's worth it.
https://codereview.chromium.org/1877143002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc (right): https://codereview.chromium.org/1877143002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:35: bool ToolbarActionsBarBubbleViews::Cancel() { On 2016/04/13 00:38:58, Devlin wrote: > Is this called when the user hits escape? yes (DCV handles the esc accelerator by activating the cancel button) https://codereview.chromium.org/1877143002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc (left): https://codereview.chromium.org/1877143002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc:55: EXPECT_EQ(HeadingString(), bubble->heading_label()->text()); On 2016/04/13 00:38:58, Devlin wrote: > thinking about this, I'd prefer to keep the text checks here. I'm sure > BubbleDialogDelegateView probably has them, too, but this also serves to test > that the delegate passes up the right string for each button. And, really, it's > just a super cheap check in a unittest, so I'd say that it's worth it. Here is my thinking: I'm not a fan of unit tests that just parrot the implementation because I don't think they add any security against regressions, they just make it twice as much manual effort to make any code changes. EXPECT_EQ(HeadingString(), bubble->GetWindowTitle()) is an example of such parroting. The cost is not in runtime but in reading and maintaining the tests. By distilling the test to the two important EXPECTs I think clarity improves. The second cost is that you have to expose content_label() just for the purposes of testing. This makes the prod code harder to grok because it's not clear where content_label() is used (even if you put "// just for testing", people may not read the comments and it adds clutter to the header file).
https://codereview.chromium.org/1877143002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc (left): https://codereview.chromium.org/1877143002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc:55: EXPECT_EQ(HeadingString(), bubble->heading_label()->text()); On 2016/04/13 15:31:03, Evan Stade wrote: > On 2016/04/13 00:38:58, Devlin wrote: > > thinking about this, I'd prefer to keep the text checks here. I'm sure > > BubbleDialogDelegateView probably has them, too, but this also serves to test > > that the delegate passes up the right string for each button. And, really, > it's > > just a super cheap check in a unittest, so I'd say that it's worth it. > > Here is my thinking: > > I'm not a fan of unit tests that just parrot the implementation because I don't > think they add any security against regressions, they just make it twice as much > manual effort to make any code changes. EXPECT_EQ(HeadingString(), > bubble->GetWindowTitle()) is an example of such parroting. The cost is not in > runtime but in reading and maintaining the tests. By distilling the test to the > two important EXPECTs I think clarity improves. I agree with the spirit (i.e., change detector tests are bad), but I think that there's enough intermediary steps here that make it worthwhile. For instance, I don't think we should check EXPECT_EQ(HeadingString(), delegate.GetHeadingString()), but checking the button text checks that the right text is returned and applied for the right button, which, while not complicated, is enough of a path that I think it's worth testing, and that if it ever changes, the test should be updated. > The second cost is that you have to expose content_label() just for the purposes > of testing. This makes the prod code harder to grok because it's not clear where > content_label() is used (even if you put "// just for testing", people may not > read the comments and it adds clutter to the header file). I'm a little less worried about this. For one thing, codesearch helps here since you can just check the xrefs. If you wanted, you could also rename the accessors to be content_label_for_testing(). Or, I know that sky@ often favors just friending the test class and not exposing them generally, which is also fine. And, at the end of the day, exposing const Label* content_label() const really isn't that dangerous. ;)
https://codereview.chromium.org/1877143002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc (left): https://codereview.chromium.org/1877143002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc:55: EXPECT_EQ(HeadingString(), bubble->heading_label()->text()); On 2016/04/13 16:33:40, Devlin wrote: > On 2016/04/13 15:31:03, Evan Stade wrote: > > On 2016/04/13 00:38:58, Devlin wrote: > > > thinking about this, I'd prefer to keep the text checks here. I'm sure > > > BubbleDialogDelegateView probably has them, too, but this also serves to > test > > > that the delegate passes up the right string for each button. And, really, > > it's > > > just a super cheap check in a unittest, so I'd say that it's worth it. > > > > Here is my thinking: > > > > I'm not a fan of unit tests that just parrot the implementation because I > don't > > think they add any security against regressions, they just make it twice as > much > > manual effort to make any code changes. EXPECT_EQ(HeadingString(), > > bubble->GetWindowTitle()) is an example of such parroting. The cost is not in > > runtime but in reading and maintaining the tests. By distilling the test to > the > > two important EXPECTs I think clarity improves. > > I agree with the spirit (i.e., change detector tests are bad), but I think that > there's enough intermediary steps here that make it worthwhile. For instance, I > don't think we should check > EXPECT_EQ(HeadingString(), delegate.GetHeadingString()), > but checking the button text checks that the right text is returned and applied > for the right button, which, while not complicated, is enough of a path that I > think it's worth testing, and that if it ever changes, the test should be > updated. OK, I restored the button text checks. > > > The second cost is that you have to expose content_label() just for the > purposes > > of testing. This makes the prod code harder to grok because it's not clear > where > > content_label() is used (even if you put "// just for testing", people may not > > read the comments and it adds clutter to the header file). > > I'm a little less worried about this. For one thing, codesearch helps here > since you can just check the xrefs. If you wanted, you could also rename the > accessors to be content_label_for_testing(). Or, I know that sky@ often favors > just friending the test class and not exposing them generally, which is also > fine. And, at the end of the day, exposing > const Label* content_label() const > really isn't that dangerous. ;) cs xrefs miss a lot (stuff used only on windows or mac). And just having a content_label_ member var (instead of local var) makes me think "hmm this must be referenced somewhere besides construction time", when that's false. https://codereview.chromium.org/1877143002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/extension_message_bubble_view_browsertest.cc (left): https://codereview.chromium.org/1877143002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_message_bubble_view_browsertest.cc:40: EXPECT_LE(bubble_bounds.y(), reference_bounds.bottom() + kFudgeFactor); This changed because for some reason it was testing the contents of the bubble rather than the bubble itself, but now that the title is technically part of the frame view and not the BubbleDialogDelegateView, the onscreen coordinates changed.
The CQ bit was checked by estade@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/1877143002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1877143002/80001
The CQ bit was checked by estade@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/1877143002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1877143002/100001
https://codereview.chromium.org/1877143002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right): https://codereview.chromium.org/1877143002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/browser_actions_container.cc:334: active_bubble_ = bubble; nit: I seem to recall once running into some edge case where we could call Show() and the bubble would try to show and close synchronously. If that happens, this will be wrong. Can we put the active_bubble_ assignment back above Show() so that if everything happens in one quick burst, we still track it correctly? (Or, if you're certain this is no longer a concern, that's fine too.) https://codereview.chromium.org/1877143002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc (right): https://codereview.chromium.org/1877143002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:67: ToolbarActionsBarBubbleDelegate::CLOSE_DISMISS_DEACTIVATION); The comments on DialogDelegate::Close make me a little nervous about treating this as deactivation. In particular, it says it can be called "by pressing the close button on the window or using a window manager gesture". Those seem a little to user-actiony to me. I wonder if we can instead add the close_reason logic from BubbleDelegate to BubbleDialogDelegate, so that we only assign CLOSE_DISMISS_DEACTIVATION when the reason is deactivation? It even looks like BubbleDialogDelegate has the CloseReason enum, though it's not used anywhere.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1877143002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right): https://codereview.chromium.org/1877143002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/browser_actions_container.cc:334: active_bubble_ = bubble; On 2016/04/13 22:36:53, Devlin wrote: > nit: I seem to recall once running into some edge case where we could call > Show() and the bubble would try to show and close synchronously. If that > happens, this will be wrong. Can we put the active_bubble_ assignment back > above Show() so that if everything happens in one quick burst, we still track it > correctly? > > (Or, if you're certain this is no longer a concern, that's fine too.) Done. https://codereview.chromium.org/1877143002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc (right): https://codereview.chromium.org/1877143002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:67: ToolbarActionsBarBubbleDelegate::CLOSE_DISMISS_DEACTIVATION); On 2016/04/13 22:36:53, Devlin wrote: > The comments on DialogDelegate::Close make me a little nervous about treating > this as deactivation. In particular, it says it can be called "by pressing the > close button on the window or using a window manager gesture". Those seem a > little to user-actiony to me. > > I wonder if we can instead add the close_reason logic from BubbleDelegate to > BubbleDialogDelegate, so that we only assign CLOSE_DISMISS_DEACTIVATION when the > reason is deactivation? It even looks like BubbleDialogDelegate has the > CloseReason enum, though it's not used anywhere. Well, there is no close button on this bubble, but if there were, you could figure out if it were clicked with GetBubbleFrameView()->close_button_clicked(). I'm specifically trying to get rid of CloseReason because no bubble I've found (and I've looked at tens of them) actually needs it. The reason that enum still exists is I forgot to delete it.
The CQ bit was checked by estade@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/1877143002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1877143002/120001
lgtm with some more thoughts on dismiss reason. I'll trust your judgment for what's best to do here. https://codereview.chromium.org/1877143002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc (right): https://codereview.chromium.org/1877143002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:67: ToolbarActionsBarBubbleDelegate::CLOSE_DISMISS_DEACTIVATION); On 2016/04/14 00:37:08, Evan Stade wrote: > On 2016/04/13 22:36:53, Devlin wrote: > > The comments on DialogDelegate::Close make me a little nervous about treating > > this as deactivation. In particular, it says it can be called "by pressing > the > > close button on the window or using a window manager gesture". Those seem a > > little to user-actiony to me. > > > > I wonder if we can instead add the close_reason logic from BubbleDelegate to > > BubbleDialogDelegate, so that we only assign CLOSE_DISMISS_DEACTIVATION when > the > > reason is deactivation? It even looks like BubbleDialogDelegate has the > > CloseReason enum, though it's not used anywhere. > > Well, there is no close button on this bubble, but if there were, you could > figure out if it were clicked with GetBubbleFrameView()->close_button_clicked(). > > I'm specifically trying to get rid of CloseReason because no bubble I've found > (and I've looked at tens of them) actually needs it. The reason that enum still > exists is I forgot to delete it. This bubble needs it. :) It, and the now-consumed ExtensionMessageBubbleView, are the reasons it was added. My concern here is that this is called directly from Widget::Close(), which is, in turn, called from, well, everywhere. So unless the BubbleDialogDelegateView is sure to call into Cancel() first, we'll be likely to miscategorize a close as due to dismissal. If it were me, I might recommend pulling the functionality to check for dismissal (i.e., OnWidgetActivationChanged()) into this class. But you're probably more familiar with the bubble code than I am, so if you're confident that Cancel() is called for user action close cases, I'll trust you on it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1877143002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc (right): https://codereview.chromium.org/1877143002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:67: ToolbarActionsBarBubbleDelegate::CLOSE_DISMISS_DEACTIVATION); On 2016/04/14 01:41:41, Devlin wrote: > On 2016/04/14 00:37:08, Evan Stade wrote: > > On 2016/04/13 22:36:53, Devlin wrote: > > > The comments on DialogDelegate::Close make me a little nervous about > treating > > > this as deactivation. In particular, it says it can be called "by pressing > > the > > > close button on the window or using a window manager gesture". Those seem a > > > little to user-actiony to me. > > > > > > I wonder if we can instead add the close_reason logic from BubbleDelegate to > > > BubbleDialogDelegate, so that we only assign CLOSE_DISMISS_DEACTIVATION when > > the > > > reason is deactivation? It even looks like BubbleDialogDelegate has the > > > CloseReason enum, though it's not used anywhere. > > > > Well, there is no close button on this bubble, but if there were, you could > > figure out if it were clicked with > GetBubbleFrameView()->close_button_clicked(). > > > > I'm specifically trying to get rid of CloseReason because no bubble I've found > > (and I've looked at tens of them) actually needs it. The reason that enum > still > > exists is I forgot to delete it. > > This bubble needs it. :) It, and the now-consumed ExtensionMessageBubbleView, > are the reasons it was added. My concern here is that this is called directly > from Widget::Close(), which is, in turn, called from, well, everywhere. But is "everywhere" more likely to be like deactivation or like explicit close? > So > unless the BubbleDialogDelegateView is sure to call into Cancel() first, we'll > be likely to miscategorize a close as due to dismissal. If it were me, I might > recommend pulling the functionality to check for dismissal (i.e., > OnWidgetActivationChanged()) into this class. But you're probably more familiar > with the bubble code than I am, so if you're confident that Cancel() is called > for user action close cases, I'll trust you on it. It depends on what you mean by user action closes, but for instances where users actually interact with the bubble and tell it to close, yes.
The CQ bit was checked by estade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org Link to the patchset: https://codereview.chromium.org/1877143002/#ps120001 (title: ".")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1877143002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1877143002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Convert ToolbarActionsBarBubbleViews to BubbleDialogDelegate While we're at it, fix a bug where the dismiss button was treated identically to the confirm button. BUG=585312 ========== to ========== Convert ToolbarActionsBarBubbleViews to BubbleDialogDelegate While we're at it, fix a bug where the dismiss button was treated identically to the confirm button. BUG=585312 Committed: https://crrev.com/dc0d234f7c87d29e9d070249a5dd601141faff5b Cr-Commit-Position: refs/heads/master@{#387219} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/dc0d234f7c87d29e9d070249a5dd601141faff5b Cr-Commit-Position: refs/heads/master@{#387219} |
