Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10656019:
Use cached composition bounds for firstRectForCharacterRange (Closed)
|
Created:
8 years, 6 months ago by Seigo Nonaka Modified:
8 years, 5 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, jochen+watch-content_chromium.org, jam, penghuang+watch_chromium.org, joi+watch-content_chromium.org, darin-cc_chromium.org, horo, Robert Sesek Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionUse cached composition bounds for firstRectForCharacterRange if possible.
This CL fixes(or reduces) mac dead-lock issue.
Browser process uses synchronous IPC for firstRectForCharacterRange request, but also renderer process uses synchronous IPC for some reasons.
This CL changes from using synchronous IPC to using cached composition character rectangle in browser process if possible.
BUG=115920
TEST=try bot and manually done on Lion
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=147843
Patch Set 1 : CL for review #Patch Set 2 : Refine comments #Patch Set 3 : fix range state #Patch Set 4 : WIP #Patch Set 5 : WIP #Patch Set 6 : WIP: remaining task is unittest #Patch Set 7 : WIP: need to fix style violation #Patch Set 8 : CL for review #Patch Set 9 : Fix rebase miss #Patch Set 10 : Fix style nits #Patch Set 11 : Use sync call if there are no info. #
Total comments: 12
Patch Set 12 : Address comments #Patch Set 13 : leave todo comment #
Total comments: 10
Patch Set 14 : Address comments #
Total comments: 6
Patch Set 15 : Address comments #
Total comments: 2
Patch Set 16 : Uses cached caret position if possible #Patch Set 17 : Fix style nits #
Total comments: 12
Patch Set 18 : rebase #Patch Set 19 : address comments #
Messages
Total messages: 27 (0 generated)
Sorry, please stop review. After discussion with horo@ and kinaba@, we will need more investigation why the synchronous IPC is timed out. Thanks.
I think they are two different issues. The approach I proposed should still be reasonable even if the synchronous IPC time out issue gets solved (though I doubt if it could be solved completely). For this CL, I'd prefer to limit the change inside render_widget_host_view_mac.mm, without touching text_input_client_mac.*. You may notice that TextInputClientMac is a singleton, so caching composition information inside it is not appropriate, even though it may work without problem. For the synchronous IPC issue, the key problem is that the synchronous calls might be very slow (e.g., tens or hundreds of milliseconds) in many cases, if the web site is busy running some javascript code. It's not acceptable to block the browser process for such a long time in very high frequency (several times a second). On 2012/06/26 01:10:15, Seigo Nonaka wrote: > Sorry, please stop review. > > After discussion with horo@ and kinaba@, we will need more investigation why the > synchronous IPC is timed out. > > Thanks.
Sorry for late response. On 2012/06/26 02:48:57, James Su wrote: > I think they are two different issues. The approach I proposed should still be > reasonable even if the synchronous IPC time out issue gets solved (though I > doubt if it could be solved completely). > > For this CL, I'd prefer to limit the change inside > render_widget_host_view_mac.mm, without touching text_input_client_mac.*. You > may notice that TextInputClientMac is a singleton, so caching composition > information inside it is not appropriate, even though it may work without > problem. > Sure, I changed to just removing IPC call for TextInputClient. > For the synchronous IPC issue, the key problem is that the synchronous calls > might be very slow (e.g., tens or hundreds of milliseconds) in many cases, if > the web site is busy running some javascript code. It's not acceptable to block > the browser process for such a long time in very high frequency (several times a > second). I found that the root cause of the issue is synchronous call dead-locking not performance related one, so we can't solve without removing browser side synchronous call. > > On 2012/06/26 01:10:15, Seigo Nonaka wrote: > > Sorry, please stop review. > > > > After discussion with horo@ and kinaba@, we will need more investigation why > the > > synchronous IPC is timed out. > > > > Thanks. Thanks Seigo
Sorry this CL does not works well on Google Docs. I investigate the reason.
I'll review your CL asap. But please fix one thing first: As far as I understand, NSTextInputClient's firstRectForCharacterRange: method is not just for input methods, other text service applications like Dictionary may also call it with an arbitrary character range. So we shouldn't simply remove the synchronous call, which will break the functionality. Instead, we should continue to use the old synchronous call if the character range is outside the composition range, that we cannot return the result immediately. On 2012/07/03 07:08:09, Seigo Nonaka wrote: > Sorry for late response. > > On 2012/06/26 02:48:57, James Su wrote: > > I think they are two different issues. The approach I proposed should still be > > reasonable even if the synchronous IPC time out issue gets solved (though I > > doubt if it could be solved completely). > > > > For this CL, I'd prefer to limit the change inside > > render_widget_host_view_mac.mm, without touching text_input_client_mac.*. You > > may notice that TextInputClientMac is a singleton, so caching composition > > information inside it is not appropriate, even though it may work without > > problem. > > > Sure, I changed to just removing IPC call for TextInputClient. > > > For the synchronous IPC issue, the key problem is that the synchronous calls > > might be very slow (e.g., tens or hundreds of milliseconds) in many cases, if > > the web site is busy running some javascript code. It's not acceptable to > block > > the browser process for such a long time in very high frequency (several times > a > > second). > I found that the root cause of the issue is synchronous call dead-locking not > performance related one, so we can't solve without removing browser side > synchronous call. > > > > > On 2012/06/26 01:10:15, Seigo Nonaka wrote: > > > Sorry, please stop review. > > > > > > After discussion with horo@ and kinaba@, we will need more investigation why > > the > > > synchronous IPC is timed out. > > > > > > Thanks. > > Thanks > > Seigo
Sure, done. On 2012/07/03 10:33:16, James Su wrote: > I'll review your CL asap. But please fix one thing first: > As far as I understand, NSTextInputClient's firstRectForCharacterRange: method > is not just for input methods, other text service applications like Dictionary > may also call it with an arbitrary character range. So we shouldn't simply > remove the synchronous call, which will break the functionality. > Instead, we should continue to use the old synchronous call if the character > range is outside the composition range, that we cannot return the result > immediately. > > On 2012/07/03 07:08:09, Seigo Nonaka wrote: > > Sorry for late response. > > > > On 2012/06/26 02:48:57, James Su wrote: > > > I think they are two different issues. The approach I proposed should still > be > > > reasonable even if the synchronous IPC time out issue gets solved (though I > > > doubt if it could be solved completely). > > > > > > For this CL, I'd prefer to limit the change inside > > > render_widget_host_view_mac.mm, without touching text_input_client_mac.*. > You > > > may notice that TextInputClientMac is a singleton, so caching composition > > > information inside it is not appropriate, even though it may work without > > > problem. > > > > > Sure, I changed to just removing IPC call for TextInputClient. > > > > > For the synchronous IPC issue, the key problem is that the synchronous calls > > > might be very slow (e.g., tens or hundreds of milliseconds) in many cases, > if > > > the web site is busy running some javascript code. It's not acceptable to > > block > > > the browser process for such a long time in very high frequency (several > times > > a > > > second). > > I found that the root cause of the issue is synchronous call dead-locking not > > performance related one, so we can't solve without removing browser side > > synchronous call. > > > > > > > > On 2012/06/26 01:10:15, Seigo Nonaka wrote: > > > > Sorry, please stop review. > > > > > > > > After discussion with horo@ and kinaba@, we will need more investigation > why > > > the > > > > synchronous IPC is timed out. > > > > > > > > Thanks. > > > > Thanks > > > > Seigo
http://codereview.chromium.org/10656019/diff/18015/content/browser/renderer_h... File content/browser/renderer_host/render_widget_host_view_mac.h (right): http://codereview.chromium.org/10656019/diff/18015/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_mac.h:454: gfx::Rect caret_rect_; caret_rect_ is not used at all. http://codereview.chromium.org/10656019/diff/18015/content/browser/renderer_h... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): http://codereview.chromium.org/10656019/diff/18015/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_mac.mm:1056: if (bounds[idx].x() > bounds[idx + 1].x()) { I'm wondering if it works for text with complex layout characters, in which the characters visual position maybe swapped. http://codereview.chromium.org/10656019/diff/18015/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_mac.mm:1067: DCHECK(range.end() < composition_bounds_.size()); start and end may equal to composition_bounds_.size(). http://codereview.chromium.org/10656019/diff/18015/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_mac.mm:1083: composition_bounds_[range.start()].height()); I think we should return the union rect of all rects within [start, end), instead of just a rect calculated by start and end. http://codereview.chromium.org/10656019/diff/18015/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_mac.mm:1088: if (composition_range_.is_empty()) I think we probably should handle the empty case, as long as it's inside the composition_range_. Maybe we should return a zero width rect for an empty range. http://codereview.chromium.org/10656019/diff/18015/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_mac.mm:1095: request_range.start() >= composition_range_.end()) request_rang.end() > composition_range_.end() ?
http://codereview.chromium.org/10656019/diff/18015/content/browser/renderer_h... File content/browser/renderer_host/render_widget_host_view_mac.h (right): http://codereview.chromium.org/10656019/diff/18015/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_mac.h:454: gfx::Rect caret_rect_; On 2012/07/04 01:44:04, James Su wrote: > caret_rect_ is not used at all. Done. http://codereview.chromium.org/10656019/diff/18015/content/browser/renderer_h... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): http://codereview.chromium.org/10656019/diff/18015/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_mac.mm:1056: if (bounds[idx].x() > bounds[idx + 1].x()) { Yes, this logic does not work for bidi characters. However at this moment, this function is used only for composition rectangle. So if there is some tool which composes bidi characters, this should be fixed, but I think it's okay to leave TODO. What do you think? And I'm not sure about the correct behavior of firstRectForCharRange for bidi mixed character. On 2012/07/04 01:44:04, James Su wrote: > I'm wondering if it works for text with complex layout characters, in which the > characters visual position maybe swapped. http://codereview.chromium.org/10656019/diff/18015/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_mac.mm:1067: DCHECK(range.end() < composition_bounds_.size()); Done for range.end(). start point is inclusive, so we can't accept it. On 2012/07/04 01:44:04, James Su wrote: > start and end may equal to composition_bounds_.size(). http://codereview.chromium.org/10656019/diff/18015/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_mac.mm:1083: composition_bounds_[range.start()].height()); On 2012/07/04 01:44:04, James Su wrote: > I think we should return the union rect of all rects within [start, end), > instead of just a rect calculated by start and end. Done. http://codereview.chromium.org/10656019/diff/18015/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_mac.mm:1088: if (composition_range_.is_empty()) This check means that we don't have any cached composition boundaries. If you worry about request_range empty case, it's okay because we already handled. On 2012/07/04 01:44:04, James Su wrote: > I think we probably should handle the empty case, as long as it's inside the > composition_range_. Maybe we should return a zero width rect for an empty range. http://codereview.chromium.org/10656019/diff/18015/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_mac.mm:1095: request_range.start() >= composition_range_.end()) The end is exclusive, so treating out of range is safer I think. On 2012/07/04 01:44:04, James Su wrote: > request_rang.end() > composition_range_.end() ?
http://codereview.chromium.org/10656019/diff/17010/content/browser/renderer_h... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): http://codereview.chromium.org/10656019/diff/17010/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_mac.mm:1054: // TODO(nona): Bidi support. Yes, Bidi supporting is a big problem, besides it, as I said in earlier comment, complex text layout is also a problem. For example, in Hindi (and many other languages), a vowel may be displayed before a consonant even though it's after the consonant in the buffer. So I think it's probably more reliable to check y value instead of x. For example, we may use the average (or max) height of rects in composition_bounds_ as a reference threshold value, then we can consider it's the start of a new line, if the y offset is larger than a certain percentage (e.g. 75%) of the threshold value. http://codereview.chromium.org/10656019/diff/17010/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_mac.mm:1067: DCHECK(range.start() < composition_bounds_.size()); I'm still wondering if it's possible that an IME wants to know the end position of a composition range, so it calls this method with rang.start() == rang.end() == composition_bounds_.size() ? http://codereview.chromium.org/10656019/diff/17010/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_mac.mm:1090: if (composition_range_.is_empty()) I think it's better to return correct rect for an empty range, if the range is inside the composition range. http://codereview.chromium.org/10656019/diff/17010/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_mac.mm:1097: request_range.start() >= composition_range_.end()) request_range.start() may equal to composition_range_.end(), if the ime does want to know the end position of the composition string. http://codereview.chromium.org/10656019/diff/17010/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_mac.mm:2763: *actualRange = theRange; Can you please help fix the logic of actualRange? We need to set it correctly.
http://codereview.chromium.org/10656019/diff/17010/content/browser/renderer_h... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): http://codereview.chromium.org/10656019/diff/17010/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_mac.mm:1054: // TODO(nona): Bidi support. On 2012/07/05 01:44:26, James Su wrote: > Yes, Bidi supporting is a big problem, besides it, as I said in earlier comment, > complex text layout is also a problem. For example, in Hindi (and many other > languages), a vowel may be displayed before a consonant even though it's after > the consonant in the buffer. > So I think it's probably more reliable to check y value instead of x. > For example, we may use the average (or max) height of rects in > composition_bounds_ as a reference threshold value, then we can consider it's > the start of a new line, if the y offset is larger than a certain percentage > (e.g. 75%) of the threshold value. Done. http://codereview.chromium.org/10656019/diff/17010/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_mac.mm:1067: DCHECK(range.start() < composition_bounds_.size()); On 2012/07/05 01:44:26, James Su wrote: > I'm still wondering if it's possible that an IME wants to know the end position > of a composition range, so it calls this method with rang.start() == rang.end() > == composition_bounds_.size() ? Done. http://codereview.chromium.org/10656019/diff/17010/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_mac.mm:1090: if (composition_range_.is_empty()) composition_range_.is_empty() == true means there are no cacned composition rectangles. So we can't construct any rectangle from any range. On 2012/07/05 01:44:26, James Su wrote: > I think it's better to return correct rect for an empty range, if the range is > inside the composition range. http://codereview.chromium.org/10656019/diff/17010/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_mac.mm:1097: request_range.start() >= composition_range_.end()) On 2012/07/05 01:44:26, James Su wrote: > request_range.start() may equal to composition_range_.end(), if the ime does > want to know the end position of the composition string. Done. http://codereview.chromium.org/10656019/diff/17010/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_mac.mm:2763: *actualRange = theRange; Done for chached rectangle. On 2012/07/05 01:44:26, James Su wrote: > Can you please help fix the logic of actualRange? We need to set it correctly.
http://codereview.chromium.org/10656019/diff/21006/content/browser/renderer_h... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): http://codereview.chromium.org/10656019/diff/21006/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_mac.mm:1080: DCHECK(range.start() < composition_bounds_.size()); range.start and range.end might be equal to composition_bounds_.size. http://codereview.chromium.org/10656019/diff/21006/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_mac.mm:1112: if (composition_range_.is_empty()) I think we need to cover one more case: The IME might call firstRectForCharacterRange: to get the current caret position to place its UI correctly, before having a composition text.
http://codereview.chromium.org/10656019/diff/21006/content/browser/renderer_h... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): http://codereview.chromium.org/10656019/diff/21006/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_mac.mm:1080: DCHECK(range.start() < composition_bounds_.size()); On 2012/07/06 01:28:47, James Su wrote: > range.start and range.end might be equal to composition_bounds_.size. Done. http://codereview.chromium.org/10656019/diff/21006/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_mac.mm:1112: if (composition_range_.is_empty()) Yes, it works with current logic because if this function returns InvalidRange, firstRectForCharacterRange function use IPC to retrieve requested rectangles. And I think it's better to get by IPC in that case because some web application like docs.google.com moves caret position at the same time of starting composition. On 2012/07/06 01:28:47, James Su wrote: > I think we need to cover one more case: The IME might call > firstRectForCharacterRange: to get the current caret position to place its UI > correctly, before having a composition text.
http://codereview.chromium.org/10656019/diff/21006/content/browser/renderer_h... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): http://codereview.chromium.org/10656019/diff/21006/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_mac.mm:1112: if (composition_range_.is_empty()) On 2012/07/06 02:04:42, Seigo Nonaka wrote: > Yes, it works with current logic because if this function returns InvalidRange, > firstRectForCharacterRange function use IPC to retrieve requested rectangles. > And I think it's better to get by IPC in that case because some web application > like http://docs.google.com moves caret position at the same time of starting > composition. I mean, for such case, we can cache the caret rect and simply return it if the request_range equals to the selection range (and if it's empty). So that we can get rid of more sync IPC calls. Please help verify if it's a usual case. > > On 2012/07/06 01:28:47, James Su wrote: > > I think we need to cover one more case: The IME might call > > firstRectForCharacterRange: to get the current caret position to place its UI > > correctly, before having a composition text. > http://codereview.chromium.org/10656019/diff/19011/content/browser/renderer_h... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): http://codereview.chromium.org/10656019/diff/19011/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_mac.mm:1081: DCHECK(range.end() < composition_bounds_.size()); <=
http://codereview.chromium.org/10656019/diff/21006/content/browser/renderer_h... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): http://codereview.chromium.org/10656019/diff/21006/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_mac.mm:1112: if (composition_range_.is_empty()) On 2012/07/06 02:17:23, James Su wrote: > On 2012/07/06 02:04:42, Seigo Nonaka wrote: > > Yes, it works with current logic because if this function returns > InvalidRange, > > firstRectForCharacterRange function use IPC to retrieve requested rectangles. > > And I think it's better to get by IPC in that case because some web > application > > like http://docs.google.com moves caret position at the same time of starting > > composition. > I mean, for such case, we can cache the caret rect and simply return it if the > request_range equals to the selection range (and if it's empty). So that we can > get rid of more sync IPC calls. > Please help verify if it's a usual case. > > > > > On 2012/07/06 01:28:47, James Su wrote: > > > I think we need to cover one more case: The IME might call > > > firstRectForCharacterRange: to get the current caret position to place its > UI > > > correctly, before having a composition text. > > > Done. http://codereview.chromium.org/10656019/diff/19011/content/browser/renderer_h... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): http://codereview.chromium.org/10656019/diff/19011/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_mac.mm:1081: DCHECK(range.end() < composition_bounds_.size()); On 2012/07/06 02:17:23, James Su wrote: > <= Done.
lgtm
suzhe@: Thank you for your review and comments. +sky Hi sky@, could you review this CL as a owner of content/browser? Thanks.
suzhe@: Thank you for your review and comments. +sky Hi sky@, could you review this CL as a owner of content/browser? Thanks.
sky: ping, just in case. I'm sorry if you are not right person for this owner review. In that case, please introduce the right person to me? Thank you.
I'm not a good person for this review. I recommend thakis.
sky: thank you for your recommendation. +thakis Hi thakis, could you check this CL? Thanks.
lgtm Sorry for the delay, I was on vacation. You should never have to wait more than a day on a review -- ping after one day, and try to find another reviewer else (rsesek would've been a good reviewer for example). rsesek: fyi this CL and http://code.google.com/p/chromium/issues/detail?id=115920#c83 if you haven't seen it. http://codereview.chromium.org/10656019/diff/18017/content/browser/renderer_h... File content/browser/renderer_host/render_widget_host_view_mac.h (right): http://codereview.chromium.org/10656019/diff/18017/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_mac.h:340: // point to |line_breaking_point|. The |line_breaking_point| is valid only if s/line_breaking_point/line_break_point/ http://codereview.chromium.org/10656019/diff/18017/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_mac.h:342: bool GetLineBreakIdx(const std::vector<gfx::Rect>& bounds, nit: No abbreviations in method names http://codereview.chromium.org/10656019/diff/18017/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_mac.h:453: // The current composition character range and it's bounds. s/it's/its/ http://codereview.chromium.org/10656019/diff/18017/content/browser/renderer_h... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): http://codereview.chromium.org/10656019/diff/18017/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_mac.mm:1134: NSRange range, nit: indent arguments 4 http://codereview.chromium.org/10656019/diff/18017/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_mac.mm:1136: NSRange* actual_range) { Add a comment "// This exists to make IMEs more responsive, see http://crbug.com/115920" http://codereview.chromium.org/10656019/diff/18017/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_mac.mm:1148: ConvertCharacterRangeToCompositionRange(ui::Range(range)); nit: indent continuations 2 more
Thank you very much. I will submit after checking try bots just in case. http://codereview.chromium.org/10656019/diff/18017/content/browser/renderer_h... File content/browser/renderer_host/render_widget_host_view_mac.h (right): http://codereview.chromium.org/10656019/diff/18017/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_mac.h:340: // point to |line_breaking_point|. The |line_breaking_point| is valid only if On 2012/07/21 16:43:27, Nico wrote: > s/line_breaking_point/line_break_point/ Done. http://codereview.chromium.org/10656019/diff/18017/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_mac.h:342: bool GetLineBreakIdx(const std::vector<gfx::Rect>& bounds, On 2012/07/21 16:43:27, Nico wrote: > nit: No abbreviations in method names Done. http://codereview.chromium.org/10656019/diff/18017/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_mac.h:453: // The current composition character range and it's bounds. On 2012/07/21 16:43:27, Nico wrote: > s/it's/its/ Done. http://codereview.chromium.org/10656019/diff/18017/content/browser/renderer_h... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): http://codereview.chromium.org/10656019/diff/18017/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_mac.mm:1134: NSRange range, On 2012/07/21 16:43:27, Nico wrote: > nit: indent arguments 4 Done. http://codereview.chromium.org/10656019/diff/18017/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_mac.mm:1136: NSRange* actual_range) { On 2012/07/21 16:43:27, Nico wrote: > Add a comment "// This exists to make IMEs more responsive, see > http://crbug.com/115920%22 Done. http://codereview.chromium.org/10656019/diff/18017/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_mac.mm:1148: ConvertCharacterRangeToCompositionRange(ui::Range(range)); On 2012/07/21 16:43:27, Nico wrote: > nit: indent continuations 2 more Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nona@chromium.org/10656019/35003
Change committed as 147843
Hi nona, this broke the mac shared build: http://build.chromium.org/p/chromium.fyi/builders/Chromium%20Mac%2010.6%20Sha... Can you take a look?
…actually, I'll just send you a fix. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
