Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1301173002:
Avoid sync IPCs for firstRectForCharacterRange/attributedSubstringForProposedRange. (Closed)
|
Created:
5 years, 4 months ago by Shu Chen Modified:
5 years, 3 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, shuchen+watch_chromium.org, jam, nona+watch_chromium.org, darin-cc_chromium.org, James Su Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAvoid sync IPCs for firstRectForCharacterRange/attributedSubstringForProposedRange.
For firstRectForCharacterRange, always uses the cached rect. And it will make
IME get the wrong/old rect to position its suggestion window.
For attributedSubstringForProposedRange, always uses composition or
selection text. And it will drop the string format (fonts/color/etc.) information.
This cl is a trade-off between making IME behave properly and avoid freezing UI.
I try to make this change as small as possible so that it can be easily
rolled back for potential regressions.
The changes of cleanning up the IPC defitions and code in
TextInputClientMac will be done in a separated cl.
BUG=121917, 473850
TEST=Verified bug not repro on local build.
Committed: https://crrev.com/bc6b777b385ff691d69479691cd259215cb32e4f
Cr-Commit-Position: refs/heads/master@{#345828}
Patch Set 1 #
Total comments: 6
Patch Set 2 : addressed yukawa@'s comments. #
Total comments: 12
Patch Set 3 : addressed comments. #Patch Set 4 : no need to convert to UTF32 for string truncation. #
Total comments: 2
Patch Set 5 : nits. #
Total comments: 4
Patch Set 6 : avoid leak. #
Total comments: 1
Patch Set 7 : avoid string copies. #
Total comments: 6
Patch Set 8 : nits. #Patch Set 9 : fixed test failures #
Messages
Total messages: 46 (8 generated)
shuchen@chromium.org changed reviewers: + asvitkine@chromium.org, yukawa@chromium.org
asvitkine@/yukawa@, can you please review this cl? Thanks
I think firstRectForCharacterRange part is OK. As for attributedSubstringForProposedRange, I have no idea if dropping string format (fonts/color/etc.) information may cause any issues with major IMEs or not. James or Horo-san (horo@) may have some insight. One possible approach might be to land firstRectForCharacterRange part first to see if it mitigates http://crbug.com/473850 or not.
On 2015/08/20 08:34:31, yukawa wrote: > I think firstRectForCharacterRange part is OK. As for > attributedSubstringForProposedRange, I have no idea if dropping string format > (fonts/color/etc.) information may cause any issues with major IMEs or not. > James or Horo-san (horo@) may have some insight. > > One possible approach might be to land firstRectForCharacterRange part first to > see if it mitigates http://crbug.com/473850 or not. I've verified locally that ctrl+x will trigger both firstRectForCharacterRange and attributedSubstringForProposedRange. And both of them caused sync IPC timeout (~1.5 seconds for each). So in terms of fixing the bug, those 2 changes are both necessary.
https://codereview.chromium.org/1301173002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/1301173002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_mac.mm:1196: void RenderWidgetHostViewMac::SelectionChanged(const base::string16& text, Just out of curiosity, if we send NSAttributedString rather than base::string16 from the rendered to the browser process, is it possible to return the string with the format information in attributedSubstringForProposedRange? https://codereview.chromium.org/1301173002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_mac.mm:2904: } else if (!renderWidgetHostView_->selection_range().is_empty()) { I think we can take advantage of |RenderWidgetHostViewBase::selection_text_| and |RenderWidgetHostViewBase::selection_text_offset_| here. In order to support IMR_RECONVERTSTRING on Windows, |RenderWidgetHostViewBase::selection_text_| generally contains preceding and trailing characters around the actual text selection, which means that we can support a bit wider range than the selected range. https://codereview.chromium.org/1301173002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_mac.mm:2909: if (expected_range.start() <= range.location && Don't we need to return nil when there is no intersection between |expected_range| and |range|? Also don't we need to update |range| (or |actualRange|) with |expected_range|? https://developer.apple.com/library/mac/documentation/Cocoa/Reference/NSTextI...:
https://codereview.chromium.org/1301173002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/1301173002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_mac.mm:1196: void RenderWidgetHostViewMac::SelectionChanged(const base::string16& text, On 2015/08/20 09:14:49, yukawa wrote: > Just out of curiosity, if we send NSAttributedString rather than base::string16 > from the rendered to the browser process, is it possible to return the string > with the format information in attributedSubstringForProposedRange? It is not easy to do that without a bigger refactoring. SelectionChanged() is for all platforms but Attributed-String is only for Mac platform. If we want to do that, it should be a separated cl. https://codereview.chromium.org/1301173002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_mac.mm:2904: } else if (!renderWidgetHostView_->selection_range().is_empty()) { On 2015/08/20 09:14:49, yukawa wrote: > I think we can take advantage of |RenderWidgetHostViewBase::selection_text_| and > |RenderWidgetHostViewBase::selection_text_offset_| here. In order to support > IMR_RECONVERTSTRING on Windows, |RenderWidgetHostViewBase::selection_text_| > generally contains preceding and trailing characters around the actual text > selection, which means that we can support a bit wider range than the selected > range. Done. https://codereview.chromium.org/1301173002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_mac.mm:2909: if (expected_range.start() <= range.location && On 2015/08/20 09:14:49, yukawa wrote: > Don't we need to return nil when there is no intersection between > |expected_range| and |range|? > > Also don't we need to update |range| (or |actualRange|) with |expected_range|? > > https://developer.apple.com/library/mac/documentation/Cocoa/Reference/NSTextI...: Done. I think we don't need to update |actualRange| because we always return the text according to |range|.
https://codereview.chromium.org/1301173002/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/1301173002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:2897: gfx::Range expected_range; How about making sure if |range| is not reversed as follows? Seems that gfx::Range has a special constructor for NSRange. const gfx::Range requested_range(range); if (requested_range.is_reversed()) return nil; https://codereview.chromium.org/1301173002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:2899: const base::string16* expected_text; Can we use StringPiece16 instead? https://codereview.chromium.org/1301173002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:2910: if (expected_range.start() > range.location || if (!expected_range.Contains(expected_range))
https://codereview.chromium.org/1301173002/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/1301173002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:2910: if (expected_range.start() > range.location || On 2015/08/21 03:46:13, yukawa wrote: > if (!expected_range.Contains(expected_range)) Correction: if (!expected_range.Contains(requested_range))
https://codereview.chromium.org/1301173002/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/1301173002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:2897: gfx::Range expected_range; On 2015/08/21 03:46:13, yukawa wrote: > How about making sure if |range| is not reversed as follows? Seems that > gfx::Range has a special constructor for NSRange. > > const gfx::Range requested_range(range); > if (requested_range.is_reversed()) > return nil; Done. https://codereview.chromium.org/1301173002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:2899: const base::string16* expected_text; On 2015/08/21 03:46:13, yukawa wrote: > Can we use StringPiece16 instead? Using StringPiece16 cannot make the code more concise. But I've revised the code to use it anyway. https://codereview.chromium.org/1301173002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:2910: if (expected_range.start() > range.location || On 2015/08/21 03:49:36, yukawa wrote: > On 2015/08/21 03:46:13, yukawa wrote: > > if (!expected_range.Contains(expected_range)) > > Correction: > if (!expected_range.Contains(requested_range)) Done.
lgtm with one minor suggestion that is basically up to you. https://codereview.chromium.org/1301173002/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/1301173002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:2899: const base::string16* expected_text; On 2015/08/21 05:17:04, Shu Chen wrote: > On 2015/08/21 03:46:13, yukawa wrote: > > Can we use StringPiece16 instead? > > Using StringPiece16 cannot make the code more concise. But I've revised the code > to use it anyway. I see. Well, sorry for asking you for different thing again but I'm a bit worried about the risk of unexpected use-after-free here if someone changes internal details of |renderWidgetHostView_->composition_range()| or |renderWidgetHostView_->selection_text()|. How about simply copying the string here? It's up to you, but I suppose string copy is acceptable, probably.
https://codereview.chromium.org/1301173002/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/1301173002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:2899: const base::string16* expected_text; On 2015/08/21 05:45:24, yukawa wrote: > On 2015/08/21 05:17:04, Shu Chen wrote: > > On 2015/08/21 03:46:13, yukawa wrote: > > > Can we use StringPiece16 instead? > > > > Using StringPiece16 cannot make the code more concise. But I've revised the > code > > to use it anyway. > > I see. Well, sorry for asking you for different thing again but I'm a bit > worried about the risk of unexpected use-after-free here if someone changes > internal details of |renderWidgetHostView_->composition_range()| or > |renderWidgetHostView_->selection_text()|. How about simply copying the string > here? It's up to you, but I suppose string copy is acceptable, probably. Sorry I don't understand. The |exptected_text| is used temporarily and synchronously. Would there be a chance that internal state is changed in a multi-threading environment while this method is executing?
https://codereview.chromium.org/1301173002/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/1301173002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:2899: const base::string16* expected_text; On 2015/08/21 05:59:49, Shu Chen wrote: > On 2015/08/21 05:45:24, yukawa wrote: > > On 2015/08/21 05:17:04, Shu Chen wrote: > > > On 2015/08/21 03:46:13, yukawa wrote: > > > > Can we use StringPiece16 instead? > > > > > > Using StringPiece16 cannot make the code more concise. But I've revised the > > code > > > to use it anyway. > > > > I see. Well, sorry for asking you for different thing again but I'm a bit > > worried about the risk of unexpected use-after-free here if someone changes > > internal details of |renderWidgetHostView_->composition_range()| or > > |renderWidgetHostView_->selection_text()|. How about simply copying the > string > > here? It's up to you, but I suppose string copy is acceptable, probably. > > Sorry I don't understand. The |exptected_text| is used temporarily and > synchronously. > Would there be a chance that internal state is changed in a multi-threading > environment while this method is executing? const std::string selection_text() { return std::string("test"); } void test1() { const std::string* ptr = &selection_text(); // |ptr| points a destroyed object. } void test2() { StringPiece str = selection_text(); // |str| points a destroyed object. } Basically we have to be careful about the return type of |selection_text()|, which I think is hard to check when reading the caller site only.
https://codereview.chromium.org/1301173002/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/1301173002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:2899: const base::string16* expected_text; On 2015/08/21 06:19:26, yukawa wrote: > On 2015/08/21 05:59:49, Shu Chen wrote: > > On 2015/08/21 05:45:24, yukawa wrote: > > > On 2015/08/21 05:17:04, Shu Chen wrote: > > > > On 2015/08/21 03:46:13, yukawa wrote: > > > > > Can we use StringPiece16 instead? > > > > > > > > Using StringPiece16 cannot make the code more concise. But I've revised > the > > > code > > > > to use it anyway. > > > > > > I see. Well, sorry for asking you for different thing again but I'm a bit > > > worried about the risk of unexpected use-after-free here if someone changes > > > internal details of |renderWidgetHostView_->composition_range()| or > > > |renderWidgetHostView_->selection_text()|. How about simply copying the > > string > > > here? It's up to you, but I suppose string copy is acceptable, probably. > > > > Sorry I don't understand. The |exptected_text| is used temporarily and > > synchronously. > > Would there be a chance that internal state is changed in a multi-threading > > environment while this method is executing? > > const std::string selection_text() { > return std::string("test"); > } > > void test1() { > const std::string* ptr = &selection_text(); > // |ptr| points a destroyed object. > } > > void test2() { > StringPiece str = selection_text(); > // |str| points a destroyed object. > } > > Basically we have to be careful about the return type of |selection_text()|, > which I think is hard to check when reading the caller site only. So, it's not the internal details but the object type. Sorry for confusing you.
https://codereview.chromium.org/1301173002/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/1301173002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:2899: const base::string16* expected_text; On 2015/08/21 06:19:26, yukawa wrote: > On 2015/08/21 05:59:49, Shu Chen wrote: > > On 2015/08/21 05:45:24, yukawa wrote: > > > On 2015/08/21 05:17:04, Shu Chen wrote: > > > > On 2015/08/21 03:46:13, yukawa wrote: > > > > > Can we use StringPiece16 instead? > > > > > > > > Using StringPiece16 cannot make the code more concise. But I've revised > the > > > code > > > > to use it anyway. > > > > > > I see. Well, sorry for asking you for different thing again but I'm a bit > > > worried about the risk of unexpected use-after-free here if someone changes > > > internal details of |renderWidgetHostView_->composition_range()| or > > > |renderWidgetHostView_->selection_text()|. How about simply copying the > > string > > > here? It's up to you, but I suppose string copy is acceptable, probably. > > > > Sorry I don't understand. The |exptected_text| is used temporarily and > > synchronously. > > Would there be a chance that internal state is changed in a multi-threading > > environment while this method is executing? > > const std::string selection_text() { > return std::string("test"); > } > > void test1() { > const std::string* ptr = &selection_text(); > // |ptr| points a destroyed object. > } > > void test2() { > StringPiece str = selection_text(); > // |str| points a destroyed object. > } > > Basically we have to be careful about the return type of |selection_text()|, > which I think is hard to check when reading the caller site only. OK. The string copy sounds a reasonable price to pay to avoid future headaches. :)
lgtm https://codereview.chromium.org/1301173002/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/1301173002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:2918: if (!base::UTF16ToUTF8( (optional) Perhaps we can use something here to avoid UTF16 -> UTF8 -> USString conversions? https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/sys_s...
https://codereview.chromium.org/1301173002/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/1301173002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:2918: if (!base::UTF16ToUTF8( On 2015/08/21 06:54:50, yukawa wrote: > (optional) Perhaps we can use something here to avoid UTF16 -> UTF8 -> USString > conversions? > > https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/sys_s... > Done.
lgtm
https://codereview.chromium.org/1301173002/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/1301173002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:2921: return [[NSAttributedString alloc] initWithString: You need to autorelease this object, otherwise you're leaking it. https://codereview.chromium.org/1301173002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:2922: base::SysUTF16ToNSString(expected_text)]; Isn't this losing the formatting? If I remember correctly (and maybe I misremember), doesn't this code also get used for "Look Up in Dictionary" platform integration? e.g. right click on a word and trigger that option in the context menu - it should highlight it in yellow and give a popup. The yellow highlight keeps the text font size / attributes. Can you check if the above still works correctly on text of different sizes with this change?
https://codereview.chromium.org/1301173002/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/1301173002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:2921: return [[NSAttributedString alloc] initWithString: On 2015/08/24 15:34:43, Alexei Svitkine (OOO Aug15-24) wrote: > You need to autorelease this object, otherwise you're leaking it. Done. https://codereview.chromium.org/1301173002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:2922: base::SysUTF16ToNSString(expected_text)]; On 2015/08/24 15:34:43, Alexei Svitkine (OOO Aug15-24) wrote: > Isn't this losing the formatting? Yes. I've explained in the cl description and in #6. > > If I remember correctly (and maybe I misremember), doesn't this code also get > used for "Look Up in Dictionary" platform integration? e.g. right click on a > word and trigger that option in the context menu - it should highlight it in > yellow and give a popup. The yellow highlight keeps the text font size / > attributes. > > Can you check if the above still works correctly on text of different sizes with > this change? I've verified the "Look Up in Dictionary" behavior is correct with this cl.
https://codereview.chromium.org/1301173002/diff/100001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/1301173002/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_view_mac.mm:2922: base::SysUTF16ToNSString(expected_text)] autorelease]; I think you can avoid a couple of temp string copies via: const base::string16* expected_text; if (!renderWidgetHostView_->composition_range().is_empty()) { ... expected_text = &markedText_; } else { ... expected_text = &renderWidgetHostView_->selection_text(); } ... const char16* bytes = &expected_text[requested_range.start() - expected_range.start()]; base::scoped_nsobject<NSObject> ns_string( [[NSString alloc] initWithBytes:bytes length:requested_range.length() encoding:NSUTF16StringEncoding]); return [[[NSAttributedString alloc] initWithString:ns_string] autorelease]; Since the selection can be a whole document (e.g. select all), I think the optimization would be worth it (with a comment).
On 2015/08/25 14:55:39, Alexei Svitkine (OOO Aug15-24) wrote: > https://codereview.chromium.org/1301173002/diff/100001/content/browser/render... > File content/browser/renderer_host/render_widget_host_view_mac.mm (right): > > https://codereview.chromium.org/1301173002/diff/100001/content/browser/render... > content/browser/renderer_host/render_widget_host_view_mac.mm:2922: > base::SysUTF16ToNSString(expected_text)] autorelease]; > I think you can avoid a couple of temp string copies via: > > const base::string16* expected_text; > if (!renderWidgetHostView_->composition_range().is_empty()) { > ... > expected_text = &markedText_; > } else { > ... > expected_text = &renderWidgetHostView_->selection_text(); > } > > ... > > const char16* bytes = > &expected_text[requested_range.start() - expected_range.start()]; > base::scoped_nsobject<NSObject> ns_string( > [[NSString alloc] initWithBytes:bytes > length:requested_range.length() > encoding:NSUTF16StringEncoding]); > return [[[NSAttributedString alloc] initWithString:ns_string] autorelease]; > > Since the selection can be a whole document (e.g. select all), I think the > optimization would be worth it (with a comment). The reason why I make the string copies was due to yukawa@'s concern in #12. I'm now not sure the ROI of the string copies.
I don't agree with the concerns in comment 12. Specifically, I don't think selection_text would get changed on another thread. In fact, if it does, a string copy without a lock would still be the wrong thing to do - i.e. the internal state of the string might be getting changed while you're copying it. But, I'm pretty sure it shouldn't be being modified from separate threads. If there's such a concern, I suggest adding DCHECKs to guard against this (i.e. that check that that member is always accessed from the same thread).
On 2015/08/25 15:04:48, Alexei Svitkine (OOO Aug15-24) wrote:
> I don't agree with the concerns in comment 12.
>
> Specifically, I don't think selection_text would get changed on another
thread.
>
> In fact, if it does, a string copy without a lock would still be the wrong
thing
> to do - i.e. the internal state of the string might be getting changed while
> you're copying it.
>
> But, I'm pretty sure it shouldn't be being modified from separate threads. If
> there's such a concern, I suggest adding DCHECKs to guard against this (i.e.
> that check that that member is always accessed from the same thread).
It is not about multi-threading. Instead, it is about temp object & return type.
The current definition: const base::string16& selection_text();
It returns a reference.
So the concern was, if the implementation is like: const base::string16&
selection_text() { return base::string16(...); }; the string copy can make it
safe and won't cause access-after-free.
However, I think a good coding practice should prevent returning a temp object
as a reference.
On 2015/08/26 01:38:17, Shu Chen wrote:
> On 2015/08/25 15:04:48, Alexei Svitkine (OOO Aug15-24) wrote:
> > I don't agree with the concerns in comment 12.
> >
> > Specifically, I don't think selection_text would get changed on another
> thread.
> >
> > In fact, if it does, a string copy without a lock would still be the wrong
> thing
> > to do - i.e. the internal state of the string might be getting changed while
> > you're copying it.
> >
> > But, I'm pretty sure it shouldn't be being modified from separate threads.
If
> > there's such a concern, I suggest adding DCHECKs to guard against this (i.e.
> > that check that that member is always accessed from the same thread).
>
> It is not about multi-threading. Instead, it is about temp object & return
type.
>
> The current definition: const base::string16& selection_text();
> It returns a reference.
> So the concern was, if the implementation is like: const base::string16&
> selection_text() { return base::string16(...); }; the string copy can make it
> safe and won't cause access-after-free.
> However, I think a good coding practice should prevent returning a temp object
> as a reference.
Right. Given the use and implementation of selection_text() is in the same file
(well, % cc/h), I think it's safe for one to rely on the implementation of the
other here.
On 2015/08/26 14:27:49, Alexei Svitkine (OOO Aug15-24) wrote:
> On 2015/08/26 01:38:17, Shu Chen wrote:
> > On 2015/08/25 15:04:48, Alexei Svitkine (OOO Aug15-24) wrote:
> > > I don't agree with the concerns in comment 12.
> > >
> > > Specifically, I don't think selection_text would get changed on another
> > thread.
> > >
> > > In fact, if it does, a string copy without a lock would still be the wrong
> > thing
> > > to do - i.e. the internal state of the string might be getting changed
while
> > > you're copying it.
> > >
> > > But, I'm pretty sure it shouldn't be being modified from separate threads.
> If
> > > there's such a concern, I suggest adding DCHECKs to guard against this
(i.e.
> > > that check that that member is always accessed from the same thread).
> >
> > It is not about multi-threading. Instead, it is about temp object & return
> type.
> >
> > The current definition: const base::string16& selection_text();
> > It returns a reference.
> > So the concern was, if the implementation is like: const base::string16&
> > selection_text() { return base::string16(...); }; the string copy can make
it
> > safe and won't cause access-after-free.
> > However, I think a good coding practice should prevent returning a temp
object
> > as a reference.
>
> Right. Given the use and implementation of selection_text() is in the same
file
> (well, % cc/h), I think it's safe for one to rely on the implementation of the
> other here.
OK. I've uploaded the new patchset to avoid string copies.
yukawa@, please raise any concern you have. Thanks
lgtm https://codereview.chromium.org/1301173002/diff/120001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/1301173002/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_view_mac.mm:2909: expected_text = &(renderWidgetHostView_->selection_text()); Nit: I'm not sure the parentheses are necessary. https://codereview.chromium.org/1301173002/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_view_mac.mm:2917: const base::char16* bytes = Nit: Add a comment about why we're doing things this way (i.e. to avoid copies) above this. https://codereview.chromium.org/1301173002/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_view_mac.mm:2922: length:bytes_len Nit: Align :'s
https://codereview.chromium.org/1301173002/diff/120001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/1301173002/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_view_mac.mm:2909: expected_text = &(renderWidgetHostView_->selection_text()); On 2015/08/26 15:09:22, Alexei Svitkine (OOO Aug15-24) wrote: > Nit: I'm not sure the parentheses are necessary. Done. https://codereview.chromium.org/1301173002/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_view_mac.mm:2917: const base::char16* bytes = On 2015/08/26 15:09:22, Alexei Svitkine (OOO Aug15-24) wrote: > Nit: Add a comment about why we're doing things this way (i.e. to avoid copies) > above this. Done. https://codereview.chromium.org/1301173002/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_view_mac.mm:2922: length:bytes_len On 2015/08/26 15:09:22, Alexei Svitkine (OOO Aug15-24) wrote: > Nit: Align :'s Done.
lgtm if Alexei's comments are addressed. nit: How about mentioning to Issue 121917 as well as Issue 473850 in CL description as BUG=121917,473850 ?
On 2015/08/26 15:22:08, yukawa wrote: > lgtm if Alexei's comments are addressed. > > nit: How about mentioning to Issue 121917 as well as Issue 473850 in CL > description as BUG=121917,473850 ? Done. Thanks for the suggestion.
The CQ bit was checked by shuchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/1301173002/#ps140001 (title: "nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1301173002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1301173002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by shuchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1301173002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1301173002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by shuchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, yukawa@chromium.org Link to the patchset: https://codereview.chromium.org/1301173002/#ps160001 (title: "fixed test failures")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1301173002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1301173002/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/bc6b777b385ff691d69479691cd259215cb32e4f Cr-Commit-Position: refs/heads/master@{#345828}
Message was sent while issue was closed.
Great, thank you for working on this. there are still other calls to TextInputClientMac::GetInstance in that file, can't they lead to hangs as well?
Message was sent while issue was closed.
On 2015/08/27 14:28:07, jabdelmalek wrote: > Great, thank you for working on this. > > there are still other calls to TextInputClientMac::GetInstance in that file, > can't they lead to hangs as well? Yes, TextInputClientMac::GetCharacterIndexAtPoint can lead to hangs. I haven't figured out a feasible way to solve it.
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:160001) has been created in https://codereview.chromium.org/1321983003/ by shuchen@chromium.org. The reason for reverting is: Caused regression: crbug.com/526611. Need to support asynchronous "Look Up in Dictionary" with attributed string by context menu. . |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
