Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2240553003:
Track text selection on the browser side (Mac) (Closed)
|
Created:
4 years, 4 months ago by EhsanK Modified:
4 years, 4 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, shuchen+watch_chromium.org, jam, sievers+watch_chromium.org, jbauman+watch_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, kalyank, danakj+watch_chromium.org, James Su, site-isolation-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTrack text selection on the browser side (Mac)
This CL will mimic the code for aura in using TextInputManager for tracking the
text selection information on the browser side. The CL also enables the corresponding
ui test in Mac.
BUG=578168, 602723
Committed: https://crrev.com/65cf5593f03c43ccaa8f97b023909040934d9ff5
Cr-Commit-Position: refs/heads/master@{#412938}
Patch Set 1 #Patch Set 2 : Fixed a compile error on X11 && !CROMEOS #Patch Set 3 : Fixed the failing tests on 'mac_chromium_rel_ng' #
Total comments: 21
Patch Set 4 : Addressing avi@'s comments #Patch Set 5 : Rebased #Patch Set 6 : Addressing erikchen@'s comments #
Total comments: 1
Patch Set 7 : Addressing erikchen@'s comment #Patch Set 8 : GetFocusedWidget() instead of GetFocusedSubView() #
Total comments: 6
Patch Set 9 : Added #ifdef #Messages
Total messages: 54 (29 generated)
The CQ bit was checked by ekaramad@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 checked by ekaramad@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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
ekaramad@chromium.org changed reviewers: + avi@chromium.org, creis@chromium.org, kenrb@chromium.org, nasko@chromium.org
PTAL. https://codereview.chromium.org/2240553003/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/2240553003/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:964: selection_text_ = selection->text; These are also still used by android, so I might as well keep them here until they are fully removed from the base class.
Looks reasonable; I want Erik to double-check it. https://codereview.chromium.org/2240553003/diff/40001/chrome/browser/renderer... File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/2240553003/diff/40001/chrome/browser/renderer... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:607: ui::DomCode::US_A, ui::VKEY_A, true, false, false, false); Why is control-A supposed to work on the Mac? On the Mac it's command-A. https://codereview.chromium.org/2240553003/diff/40001/content/browser/rendere... File content/browser/renderer_host/text_input_manager.cc (right): https://codereview.chromium.org/2240553003/diff/40001/content/browser/rendere... content/browser/renderer_host/text_input_manager.cc:100: DCHECK(pos + n <= selection->text.length()) DCHECK_LE https://codereview.chromium.org/2240553003/diff/40001/content/browser/rendere... File content/browser/renderer_host/text_input_manager.h (right): https://codereview.chromium.org/2240553003/diff/40001/content/browser/rendere... content/browser/renderer_host/text_input_manager.h:114: // Otherwise reutrns false. returns
avi@chromium.org changed reviewers: + erikchen@chromium.org
https://codereview.chromium.org/2240553003/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/2240553003/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:926: if (!GetTextInputManager()) Can you describe a situation where this conditional can evaluate to true? Should this be DCHECK_EQ(GetTextInputManager(), text_input_manager)? https://codereview.chromium.org/2240553003/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:930: if (is_guest_view_hack_) { This code looks really similar to the logic in RenderWidgetHostViewAura::OnTextSelectionChanged. Would it make sense to pull out a common method? https://codereview.chromium.org/2240553003/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:953: GetTextInputManager()->GetTextSelection(focused_view); The logic for GetTextInputManager()->GetSelectedText() also requires calling GetTextSelection(). Should GetSelectedText() be a method on TextInputManager::TextSelection instead? https://codereview.chromium.org/2240553003/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:964: selection_text_ = selection->text; On 2016/08/12 05:07:54, EhsanK wrote: > These are also still used by android, so I might as well keep them here until > they are fully removed from the base class. Why aren't we updating the methods in render_widget_host_view_base.cc to use the AURA path for mac instead? https://codereview.chromium.org/2240553003/diff/40001/content/browser/rendere... File content/browser/renderer_host/text_input_manager.cc (right): https://codereview.chromium.org/2240553003/diff/40001/content/browser/rendere... content/browser/renderer_host/text_input_manager.cc:102: if (pos >= selection->text.length()) { Is it ever possible for an instance of TextSelection to contain invalid parameters? If so, we shouldn't have DCHECKs. If not, we shouldn't have a conditional. The code pattern: """ if (...) { NOTREACHED() } """ is a red flag.
The CQ bit was checked by ekaramad@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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Patchset #5 (id:80001) has been deleted
Patchset #5 (id:100001) has been deleted
The CQ bit was checked by ekaramad@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...
Thanks for the reviews and sorry it took a bit longer to respond (PD days). Please take another look. https://codereview.chromium.org/2240553003/diff/40001/chrome/browser/renderer... File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/2240553003/diff/40001/chrome/browser/renderer... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:607: ui::DomCode::US_A, ui::VKEY_A, true, false, false, false); On 2016/08/12 15:53:10, Avi wrote: > Why is control-A supposed to work on the Mac? On the Mac it's command-A. Very good catch! You are right and I was not selecting the whole text on Mac. But the test was still passing... So here is what happended. I thought I am looking at the selected text, i.e., highlighted. But I was looking at the whole text around selection which was the <input>.value. Since sending Ctrl+a puts the cursor at the beginning of <input>, and on aura it selects all, the length I was checking which was the |selection_text_| (in RWHVBase) was the same in both platforms. I do not have to change this part since we are testing tracking, but just to be compatible with the original purpose of the test, I changed the helper method to return the selection range's length instead (the highlighted text) and I am now passing Command + A on Mac. https://codereview.chromium.org/2240553003/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/2240553003/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:926: if (!GetTextInputManager()) On 2016/08/12 19:33:43, erikchen wrote: > Can you describe a situation where this conditional can evaluate to true? Should > this be DCHECK_EQ(GetTextInputManager(), text_input_manager)? Good question. The reason we have this is for future usage with <guest>-views and nested WebContents. It is not 100% determined how they will integrate into this model (i.e., TextInputManager per WebContents as opposed to one per outermost WebContents and how to transition from inner to outer WebContents when we attach WebContents). Eventually, |text_input_manager| argument might get removed, or might still stay. Having said all this, I think DCHECK_EQ(GetTextInputManager(), text_input_manager) should be better since we do expect this observer call from none other than the one belonging to |render_widget_host_->delegate()|. Also this is an automatic check agains null-ness. https://codereview.chromium.org/2240553003/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:930: if (is_guest_view_hack_) { On 2016/08/12 19:33:43, erikchen wrote: > This code looks really similar to the logic in > RenderWidgetHostViewAura::OnTextSelectionChanged. Would it make sense to pull > out a common method? I added this logic to obtain the focused view into the base class. Since this relies on RenderWidgetHostView::GetRenderWidgetHost() I implemented that method for the base class which returns nullptr. The only justification for it is that we might head into pure virtual calls if the method is used somewhere else. https://codereview.chromium.org/2240553003/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:953: GetTextInputManager()->GetTextSelection(focused_view); On 2016/08/12 19:33:43, erikchen wrote: > The logic for GetTextInputManager()->GetSelectedText() also requires calling > GetTextSelection(). Should GetSelectedText() be a method on > TextInputManager::TextSelection instead? Acknowledged. https://codereview.chromium.org/2240553003/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:964: selection_text_ = selection->text; On 2016/08/12 19:33:43, erikchen wrote: > On 2016/08/12 05:07:54, EhsanK wrote: > > These are also still used by android, so I might as well keep them here until > > they are fully removed from the base class. > > Why aren't we updating the methods in render_widget_host_view_base.cc to use the > AURA path for mac instead? I am not quite clear on this comment, so I will explain what I understand wait for your feedback. As I understand, with this patch we are using AURA path for Mac. The call to RenderWidgetHostViewBase::SelectionChanged will lead to a call to TextInputManager::SelectionChanged for both Mac and Aura. Then the TIM will call its observer (either RenderWidgetHostViewMac) to notify about a change in text selection; which happens similarly in aura. The only remaining issue is these member variables in the base class which we are still using in Mac. I figured it might be better to remove them all at once after implementing the same approach for Android. https://codereview.chromium.org/2240553003/diff/40001/content/browser/rendere... File content/browser/renderer_host/text_input_manager.cc (right): https://codereview.chromium.org/2240553003/diff/40001/content/browser/rendere... content/browser/renderer_host/text_input_manager.cc:100: DCHECK(pos + n <= selection->text.length()) On 2016/08/12 15:53:10, Avi wrote: > DCHECK_LE Done. https://codereview.chromium.org/2240553003/diff/40001/content/browser/rendere... content/browser/renderer_host/text_input_manager.cc:102: if (pos >= selection->text.length()) { On 2016/08/12 19:33:43, erikchen wrote: > Is it ever possible for an instance of TextSelection to contain invalid > parameters? > > If so, we shouldn't have DCHECKs. > If not, we shouldn't have a conditional. > > The code pattern: > """ > if (...) { > NOTREACHED() > } > """ > is a red flag. I am not sure but the selection information are coming from the renderer side, so they could be wrong maybe? I cannot DCHECK and not return early since it might throw exception in text.substr() later on. So, if DCHECK is incorrect here, I think I should just early return if the range values are invalid. WDYT? https://codereview.chromium.org/2240553003/diff/40001/content/browser/rendere... File content/browser/renderer_host/text_input_manager.h (right): https://codereview.chromium.org/2240553003/diff/40001/content/browser/rendere... content/browser/renderer_host/text_input_manager.h:114: // Otherwise reutrns false. On 2016/08/12 15:53:10, Avi wrote: > returns Done.
Updating a comment. https://codereview.chromium.org/2240553003/diff/40001/chrome/browser/renderer... File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/2240553003/diff/40001/chrome/browser/renderer... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:607: ui::DomCode::US_A, ui::VKEY_A, true, false, false, false); On 2016/08/16 13:36:44, EhsanK wrote: > On 2016/08/12 15:53:10, Avi wrote: > > Why is control-A supposed to work on the Mac? On the Mac it's command-A. > > Very good catch! You are right and I was not selecting the whole text on Mac. > But the test was still passing... > > So here is what happended. I thought I am looking at the selected text, i.e., > highlighted. But I was looking at the whole text around selection which was the > <input>.value. Since sending Ctrl+a puts the cursor at the beginning of <input>, > and on aura it selects all, the length I was checking which was the > |selection_text_| (in RWHVBase) was the same in both platforms. > > I do not have to change this part since we are testing tracking, but just to be > compatible with the original purpose of the test, I changed the helper method to > return the selection range's length instead (the highlighted text) and I am now > passing Command + A on Mac. Sorry - outdated comment. The second paragraph is incorrect. I did not change the helper method and I am looking at the |selection_text_| instead. So the Ctrl+A or Command+A part is removed. Actually, passing Ctrl+A did not work as expected.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2240553003/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/2240553003/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:964: selection_text_ = selection->text; On 2016/08/16 13:36:45, EhsanK wrote: > On 2016/08/12 19:33:43, erikchen wrote: > > On 2016/08/12 05:07:54, EhsanK wrote: > > > These are also still used by android, so I might as well keep them here > until > > > they are fully removed from the base class. > > > > Why aren't we updating the methods in render_widget_host_view_base.cc to use > the > > AURA path for mac instead? > I am not quite clear on this comment, so I will explain what I understand wait > for your feedback. > > As I understand, with this patch we are using AURA path for Mac. The call to > RenderWidgetHostViewBase::SelectionChanged will lead to a call to > TextInputManager::SelectionChanged for both Mac and Aura. Then the TIM will call > its observer (either RenderWidgetHostViewMac) to notify about a change in text > selection; which happens similarly in aura. > > The only remaining issue is these member variables in the base class which we > are still using in Mac. I figured it might be better to remove them all at once > after implementing the same approach for Android. That's fine. https://codereview.chromium.org/2240553003/diff/40001/content/browser/rendere... File content/browser/renderer_host/text_input_manager.cc (right): https://codereview.chromium.org/2240553003/diff/40001/content/browser/rendere... content/browser/renderer_host/text_input_manager.cc:102: if (pos >= selection->text.length()) { On 2016/08/16 13:36:45, EhsanK wrote: > On 2016/08/12 19:33:43, erikchen wrote: > > Is it ever possible for an instance of TextSelection to contain invalid > > parameters? > > > > If so, we shouldn't have DCHECKs. > > If not, we shouldn't have a conditional. > > > > The code pattern: > > """ > > if (...) { > > NOTREACHED() > > } > > """ > > is a red flag. > > I am not sure but the selection information are coming from the renderer side, > so they could be wrong maybe? > > I cannot DCHECK and not return early since it might throw exception in > text.substr() later on. So, if DCHECK is incorrect here, I think I should just > early return if the range values are invalid. WDYT? Yes, early return is appropriate. Short of memory corruption or someone breaking this class in the future, nothing should make a DCHECK be hit. You might be want a LOG(WARNING) instead?
PTAL. https://codereview.chromium.org/2240553003/diff/40001/content/browser/rendere... File content/browser/renderer_host/text_input_manager.cc (right): https://codereview.chromium.org/2240553003/diff/40001/content/browser/rendere... content/browser/renderer_host/text_input_manager.cc:102: if (pos >= selection->text.length()) { On 2016/08/16 16:36:22, erikchen wrote: > On 2016/08/16 13:36:45, EhsanK wrote: > > On 2016/08/12 19:33:43, erikchen wrote: > > > Is it ever possible for an instance of TextSelection to contain invalid > > > parameters? > > > > > > If so, we shouldn't have DCHECKs. > > > If not, we shouldn't have a conditional. > > > > > > The code pattern: > > > """ > > > if (...) { > > > NOTREACHED() > > > } > > > """ > > > is a red flag. > > > > I am not sure but the selection information are coming from the renderer side, > > so they could be wrong maybe? > > > > I cannot DCHECK and not return early since it might throw exception in > > text.substr() later on. So, if DCHECK is incorrect here, I think I should just > > early return if the range values are invalid. WDYT? > > Yes, early return is appropriate. Short of memory corruption or someone breaking > this class in the future, nothing should make a DCHECK be hit. You might be want > a LOG(WARNING) instead? I think the DCHECK above should also change then. I did replace both checks with early return and logs.
https://codereview.chromium.org/2240553003/diff/140001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2240553003/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:2752: tsc_config.show_on_tap_for_empty_editable = false; Sorry about the rebase + change together.
lgtm
The CQ bit was checked by ekaramad@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...
Thanks Erik for the review! PTAL. avi@: Could you please take another look at this patch? Thansk!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
To the extent that I can get my head around it, lgtm.
Thanks Avi! kenrb@: Could you please take a look?
Adding sky@ for the file 'site_per_process_text_input_browsertest.cc'. sky@: Could you please take a look? creis@ is OOO.
lgtm
ekaramad@chromium.org changed reviewers: + sky@chromium.org
Thanks Ken! Added sky@ since I apparently forgot to do so earlier. sky@: Could you please take a look? creis@ is OOO so he cannot LGTM the test file (site_per_process_text_input_browsertest.cc). Thanks!
https://codereview.chromium.org/2240553003/diff/180001/chrome/browser/rendere... File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/2240553003/diff/180001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:655: // TODO(ekaramad): Enable the following tests on other platforms when the Is there a compelling reason to move the test here? It makes the diff harder to follow.
Thanks sky@. I added some comments. Please take a look. https://codereview.chromium.org/2240553003/diff/180001/chrome/browser/rendere... File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (left): https://codereview.chromium.org/2240553003/diff/180001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:630: content::RenderWidgetHostView* view) { This test is just copied over (actually not copied just looks like it) in green (should be intact). https://codereview.chromium.org/2240553003/diff/180001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:685: ui::DomCode::US_A, ui::VKEY_A, true, false, false, false); This was removed since it was not needed nor correctly working. https://codereview.chromium.org/2240553003/diff/180001/chrome/browser/rendere... File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/2240553003/diff/180001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:655: // TODO(ekaramad): Enable the following tests on other platforms when the On 2016/08/17 20:42:52, sky wrote: > Is there a compelling reason to move the test here? It makes the diff harder to > follow. Sorry for the ugly diff. Hmmm...actually, the intent was to move TrackTextSelectionForAllFrames from inside #ifdef USE_AURA to outside #ifdef USE_AURA. I will keep this diff issue in mind for the upcoming CL which moves TrackSelectionBounds outside of USE_AURA.
Also, I'm perfectly fine with moving things around, but I would rather you don't move first so I can see easily see what changes, and then send around a review with the move and nothing else (you can even TBR that one). https://codereview.chromium.org/2240553003/diff/180001/chrome/browser/rendere... File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/2240553003/diff/180001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:655: // TODO(ekaramad): Enable the following tests on other platforms when the On 2016/08/17 21:36:00, EhsanK wrote: > On 2016/08/17 20:42:52, sky wrote: > > Is there a compelling reason to move the test here? It makes the diff harder > to > > follow. > > Sorry for the ugly diff. Hmmm...actually, the intent was to move > TrackTextSelectionForAllFrames from inside #ifdef USE_AURA to outside #ifdef > USE_AURA. > > I will keep this diff issue in mind for the upcoming CL which moves > TrackSelectionBounds outside of USE_AURA. Any chance you could add ifdefs instead? Again, it makes the review easier.
Thanks sky@ for the review and the suggestions. Please take another look. PS: I will move the tests around later in a separate CL (no modifications to the test logic) since we have two sets of #if defined(USE_AURA) now which might not be necessary later on. https://codereview.chromium.org/2240553003/diff/180001/chrome/browser/rendere... File chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc (right): https://codereview.chromium.org/2240553003/diff/180001/chrome/browser/rendere... chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:655: // TODO(ekaramad): Enable the following tests on other platforms when the On 2016/08/17 23:32:47, sky wrote: > On 2016/08/17 21:36:00, EhsanK wrote: > > On 2016/08/17 20:42:52, sky wrote: > > > Is there a compelling reason to move the test here? It makes the diff harder > > to > > > follow. > > > > Sorry for the ugly diff. Hmmm...actually, the intent was to move > > TrackTextSelectionForAllFrames from inside #ifdef USE_AURA to outside #ifdef > > USE_AURA. > > > > I will keep this diff issue in mind for the upcoming CL which moves > > TrackSelectionBounds outside of USE_AURA. > > Any chance you could add ifdefs instead? Done (assuming that I understood the suggestion correctly).
So much nicer to review. Thanks! LGTM
Thank you sky@! I will keep that in mind for the coming CLs. FYI, I will CQ after the dry-run.
The CQ bit was checked by ekaramad@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 ekaramad@chromium.org
The CQ bit was checked by ekaramad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from erikchen@chromium.org, kenrb@chromium.org, avi@chromium.org Link to the patchset: https://codereview.chromium.org/2240553003/#ps200001 (title: "Added #ifdef")
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 #9 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Track text selection on the browser side (Mac) This CL will mimic the code for aura in using TextInputManager for tracking the text selection information on the browser side. The CL also enables the corresponding ui test in Mac. BUG=578168,602723 ========== to ========== Track text selection on the browser side (Mac) This CL will mimic the code for aura in using TextInputManager for tracking the text selection information on the browser side. The CL also enables the corresponding ui test in Mac. BUG=578168,602723 Committed: https://crrev.com/65cf5593f03c43ccaa8f97b023909040934d9ff5 Cr-Commit-Position: refs/heads/master@{#412938} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/65cf5593f03c43ccaa8f97b023909040934d9ff5 Cr-Commit-Position: refs/heads/master@{#412938} |
