Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8536047:
Separate selection highlight from pango layout (Closed)
|
Created:
9 years, 1 month ago by xji Modified:
9 years ago CC:
chromium-reviews, Alexei Svitkine (slow) Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionSeparate selection highlight from pango layout, highlight selection using skia.
Cache substring bounds to avoid unnecessary g_free. Consolidate drawing functions with RenderTextWin. Remove UpdateLayout() from RenderText::SetSelectionModel().
BUG=103647
TEST=TextfieldViewModelTest; Manual test selection highlight on bidi text.
(Do not really know how to test "fi" ligature part. But since the selection
highlight is not done by using pango attribute, I would assume that solves the problem).
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112188
Patch Set 1 #Patch Set 2 : remove UpdateLayout() in RenderText::SetSelectionModel() #Patch Set 3 : fix a lint error #Patch Set 4 : save selection visual bounds to minimize g_free calls #Patch Set 5 : fix an lint error #
Total comments: 34
Patch Set 6 : address comments #
Total comments: 7
Patch Set 7 : sync #Patch Set 8 : synchistory #
Total comments: 4
Patch Set 9 : address comments #
Total comments: 6
Patch Set 10 : address comments #
Messages
Total messages: 18 (0 generated)
Hi Xiaomei, I'll review this tomorrow. Cheers, behdad On 2011-11-15, at 7:56 PM, xji@chromium.org wrote: > Reviewers: behdad1, msw, oshima, > > Description: > Separate selection highlight from pango layout, highlight selection using skia. > Consolidate drawing functions with RenderTextWin. Remove UpdateLayout() from > RenderText::SetSelectionModel(). > > BUG=103647 > TEST=TextfieldViewModelTest; Manual test selection highlight on bidi text. > (Do not really know how to test "fi" ligature part. But since the selection > highlight is not done by using pango attribute, I would assume that solves the > problem). > > Please review this at http://codereview.chromium.org/8536047/ > > SVN Base: svn://svn.chromium.org/chrome/trunk/src/ > > Affected files: > M ui/gfx/render_text.h > M ui/gfx/render_text.cc > M ui/gfx/render_text_linux.h > M ui/gfx/render_text_linux.cc > M ui/gfx/render_text_win.h > M ui/gfx/render_text_win.cc > >
ping reviewers. On 2011/11/16 01:00:03, behdad_google wrote: > Hi Xiaomei, > > I'll review this tomorrow. > > Cheers, > > behdad > > On 2011-11-15, at 7:56 PM, mailto:xji@chromium.org wrote: > > > Reviewers: behdad1, msw, oshima, > > > > Description: > > Separate selection highlight from pango layout, highlight selection using > skia. > > Consolidate drawing functions with RenderTextWin. Remove UpdateLayout() from > > RenderText::SetSelectionModel(). > > > > BUG=103647 > > TEST=TextfieldViewModelTest; Manual test selection highlight on bidi text. > > (Do not really know how to test "fi" ligature part. But since the selection > > highlight is not done by using pango attribute, I would assume that solves the > > problem). > > > > Please review this at http://codereview.chromium.org/8536047/ > > > > SVN Base: svn://svn.chromium.org/chrome/trunk/src/ > > > > Affected files: > > M ui/gfx/render_text.h > > M ui/gfx/render_text.cc > > M ui/gfx/render_text_linux.h > > M ui/gfx/render_text_linux.cc > > M ui/gfx/render_text_win.h > > M ui/gfx/render_text_win.cc > > > >
http://codereview.chromium.org/8536047/diff/12002/ui/gfx/render_text.h File ui/gfx/render_text.h (right): http://codereview.chromium.org/8536047/diff/12002/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:217: virtual std::vector<Rect> GetSubstringBounds(size_t from, size_t to); Maybe change the signature of this to: virtual void GetSubstringBounds(size_t from, size_t to, std::vector<Rect>* bounds); Given that RenderTextLinux's version already wraps a function with that signature and that returning a vector by value involves inefficient copying. http://codereview.chromium.org/8536047/diff/12002/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/8536047/diff/12002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:206: return selection_visual_bounds_; I think it would be cleaner to have a function GetSelectionBounds() that has this logic and would do the caching and call GetSubstringBounds() as a helper. http://codereview.chromium.org/8536047/diff/12002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:684: int w = PANGO_PIXELS(ranges[2 * i + 1] - ranges[2 * i]); Nit: Rename to 'width'. http://codereview.chromium.org/8536047/diff/12002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:686: rect.Offset(0, (display_rect().height() - height) / 2); Nit: Move y = (display_rect().height() - height) / 2); to outside the loop. Then, you can pass 'y' to the Rect constructor and remove the Offset() call.
http://codereview.chromium.org/8536047/diff/12002/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (left): http://codereview.chromium.org/8536047/diff/12002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:535: // TODO(xji): There's a bug in pango that it can't use two colors in one Is this no longer true? http://codereview.chromium.org/8536047/diff/12002/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/8536047/diff/12002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:41: selection_visual_bounds_.clear(); This isn't necessary. http://codereview.chromium.org/8536047/diff/12002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:196: DCHECK(from <= text().length() && to <= text().length()); This should be two separate DCHECK statements. http://codereview.chromium.org/8536047/diff/12002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:204: if (selection_visual_bounds_.empty()) Does the inefficiency or recalculating selection bounds justify the extra complexity of caching selection_visual_bounds_? I suspect it doesn't. http://codereview.chromium.org/8536047/diff/12002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:219: !(start == new_end && end == new_start)) Are the bounds always the same when the start and end are reversed? How often will that happen? I'd reduce complexity to simplify code. http://codereview.chromium.org/8536047/diff/12002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:239: void RenderTextLinux::DrawVisualText(Canvas* canvas) { Sync up with Alexei, he's re-writing this to use Skia. http://codereview.chromium.org/8536047/diff/12002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:675: pango_layout_line_get_x_ranges(current_line_, I believe this argument should be on a new line, indented by 4 spaces like the rest of the args (indent all by 4 or indent all after the open paren). http://codereview.chromium.org/8536047/diff/12002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:677: std::max(from_in_utf8, to_in_utf8), &ranges, &n_ranges); |&ranges| and |&n_ranges| should be on new lines. http://codereview.chromium.org/8536047/diff/12002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:682: for (int i = 0; i < 2 * n_ranges; ++i) { Shouldn't |i| be less than n_ranges? Otherwise we access ranges at |2 * i| when |i| is already |2 * n_ranges - 1|, which seems out of bounds. http://codereview.chromium.org/8536047/diff/12002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:684: int w = PANGO_PIXELS(ranges[2 * i + 1] - ranges[2 * i]); Is ranges guaranteed to include an extra terminating entry? Otherwise, we can't access ranges at |2 * (n_ranges - 1) + 1|. Also, can you re-use the the PANGO_PIXELS(ranges[2 * i]) calculated as |x| above? http://codereview.chromium.org/8536047/diff/12002/ui/gfx/render_text_linux.h File ui/gfx/render_text_linux.h (right): http://codereview.chromium.org/8536047/diff/12002/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:10: I don't think you need this blank line, both are system headers. http://codereview.chromium.org/8536047/diff/12002/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:107: void GetSubstringBounds(size_t from, size_t to, std::vector<Rect>* bounds); Our style guide discourages overloading function names. Can you rename this or merge it with the other GetSubstringBounds?
http://codereview.chromium.org/8536047/diff/12002/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/8536047/diff/12002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:682: for (int i = 0; i < 2 * n_ranges; ++i) { Agreed. http://codereview.chromium.org/8536047/diff/12002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:684: int w = PANGO_PIXELS(ranges[2 * i + 1] - ranges[2 * i]); On 2011/11/28 20:01:17, msw wrote: > Is ranges guaranteed to include an extra terminating entry? > Otherwise, we can't access ranges at |2 * (n_ranges - 1) + 1|. The array is guaranteed to be of length 2*n_ranges, so we're fine. > Also, can you re-use the the PANGO_PIXELS(ranges[2 * i]) calculated as |x| > above? In fact, "w = PANGO_PIXELS(ranges[2 * i + 1]) - x" is more correct rounding-wise. http://codereview.chromium.org/8536047/diff/12002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:685: Rect rect(x, 0, w, height); You may need "w-1" instead of w, depending on what the definition of w is. if the rectangle is one pixel wide, do you expect w=1 or w=0?
http://codereview.chromium.org/8536047/diff/12002/ui/gfx/render_text.h File ui/gfx/render_text.h (right): http://codereview.chromium.org/8536047/diff/12002/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:217: virtual std::vector<Rect> GetSubstringBounds(size_t from, size_t to); On 2011/11/28 19:56:14, Alexei Svitkine wrote: > Maybe change the signature of this to: > > virtual void GetSubstringBounds(size_t from, > size_t to, > std::vector<Rect>* bounds); > > Given that RenderTextLinux's version already wraps a function with that > signature and that returning a vector by value involves inefficient copying. Changed. It is generally true that changing the signature as above saves vector copying. But in RenderTextLinux, since I cached the selection bounds (to avoid unnecessary g_free), there is still vector copying involved if the (from, to) is the selection range, unless I change the signature to: virtual std::vector<Rect>* GetSubstringBounds(size_t from, size_t to, std::vector<Rect>* bounds) so that when (from, to) is the same as logical selection, return &selection_virtual_bounds_, and leave |bounds| untouched. The problem is that this API will have different behavior in Linux and Win. So I keep the API as what you suggested with extra vector copying in Linux. http://codereview.chromium.org/8536047/diff/12002/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/8536047/diff/12002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:41: selection_visual_bounds_.clear(); On 2011/11/28 20:01:17, msw wrote: > This isn't necessary. removed. http://codereview.chromium.org/8536047/diff/12002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:196: DCHECK(from <= text().length() && to <= text().length()); On 2011/11/28 20:01:17, msw wrote: > This should be two separate DCHECK statements. Done. http://codereview.chromium.org/8536047/diff/12002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:204: if (selection_visual_bounds_.empty()) On 2011/11/28 20:01:17, msw wrote: > Does the inefficiency or recalculating selection bounds justify the extra > complexity of caching selection_visual_bounds_? I suspect it doesn't. I am not worried too much about the calculation part. I am more concerned on the g_free part in CalculateStringBounds(). We constantly repaint textfield. if there is an selection and you leave the screen as is, the selection is constantly repaint, and g_free() is constantly called. Yes you are right that the code will be much simpler without caching selection visual bounds (it is patch 3). Maybe Behdad has better idea on the memory management. http://codereview.chromium.org/8536047/diff/12002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:206: return selection_visual_bounds_; On 2011/11/28 19:56:14, Alexei Svitkine wrote: > I think it would be cleaner to have a function GetSelectionBounds() that has > this logic and would do the caching and call GetSubstringBounds() as a helper. Introduced GetSelectionBounds(). http://codereview.chromium.org/8536047/diff/12002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:219: !(start == new_end && end == new_start)) On 2011/11/28 20:01:17, msw wrote: > Are the bounds always the same when the start and end are reversed? I think they are. > How often will that happen? I'd reduce complexity to simplify code. it probably is not that often. So I simplified the code without considering such case. http://codereview.chromium.org/8536047/diff/12002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:675: pango_layout_line_get_x_ranges(current_line_, On 2011/11/28 20:01:17, msw wrote: > I believe this argument should be on a new line, indented by 4 spaces like the > rest of the args (indent all by 4 or indent all after the open paren). done. but I think this is the style for function decl and definition, not func call. http://codereview.chromium.org/8536047/diff/12002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:677: std::max(from_in_utf8, to_in_utf8), &ranges, &n_ranges); On 2011/11/28 20:01:17, msw wrote: > |&ranges| and |&n_ranges| should be on new lines. Done. http://codereview.chromium.org/8536047/diff/12002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:682: for (int i = 0; i < 2 * n_ranges; ++i) { On 2011/11/28 20:01:17, msw wrote: > Shouldn't |i| be less than n_ranges? Otherwise we access ranges at |2 * i| when > |i| is already |2 * n_ranges - 1|, which seems out of bounds. Thanks for catching this! http://codereview.chromium.org/8536047/diff/12002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:684: int w = PANGO_PIXELS(ranges[2 * i + 1] - ranges[2 * i]); On 2011/11/28 19:56:14, Alexei Svitkine wrote: > Nit: Rename to 'width'. Done. http://codereview.chromium.org/8536047/diff/12002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:684: int w = PANGO_PIXELS(ranges[2 * i + 1] - ranges[2 * i]); On 2011/11/28 23:17:40, behdad_google wrote: > On 2011/11/28 20:01:17, msw wrote: > > Is ranges guaranteed to include an extra terminating entry? > > Otherwise, we can't access ranges at |2 * (n_ranges - 1) + 1|. > > The array is guaranteed to be of length 2*n_ranges, so we're fine. > > > Also, can you re-use the the PANGO_PIXELS(ranges[2 * i]) calculated as |x| > > above? > > In fact, "w = PANGO_PIXELS(ranges[2 * i + 1]) - x" is more correct > rounding-wise. hm... why "w = PANGO_PIXELS(ranges[2 * i + 1]) - x" is more correct rounding-wise? I thought the other way round. http://codereview.chromium.org/8536047/diff/12002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:685: Rect rect(x, 0, w, height); On 2011/11/28 23:17:40, behdad_google wrote: > You may need "w-1" instead of w, depending on what the definition of w is. if > the rectangle is one pixel wide, do you expect w=1 or w=0? Are you talking about skia's pixel model, such as we passed pixel width as 0 to draw a 1-pixel cursor? similarly, I need to pass 'w-1'? I am not sure here. http://codereview.chromium.org/8536047/diff/12002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:686: rect.Offset(0, (display_rect().height() - height) / 2); On 2011/11/28 19:56:14, Alexei Svitkine wrote: > Nit: Move y = (display_rect().height() - height) / 2); to outside the loop. > Then, you can pass 'y' to the Rect constructor and remove the Offset() call. Done. http://codereview.chromium.org/8536047/diff/12002/ui/gfx/render_text_linux.h File ui/gfx/render_text_linux.h (right): http://codereview.chromium.org/8536047/diff/12002/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:10: On 2011/11/28 20:01:17, msw wrote: > I don't think you need this blank line, both are system headers. Done. http://codereview.chromium.org/8536047/diff/12002/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:107: void GetSubstringBounds(size_t from, size_t to, std::vector<Rect>* bounds); On 2011/11/28 20:01:17, msw wrote: > Our style guide discourages overloading function names. > Can you rename this or merge it with the other GetSubstringBounds? changed the name to CalculateSubstringBounds.
http://codereview.chromium.org/8536047/diff/20001/ui/gfx/render_text.h File ui/gfx/render_text.h (left): http://codereview.chromium.org/8536047/diff/20001/ui/gfx/render_text.h#oldcode25 ui/gfx/render_text.h:25: const int kStrikeWidth = 2; Delete this constant since its no longer used with your patch. http://codereview.chromium.org/8536047/diff/20001/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/8536047/diff/20001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:196: selection_visual_bounds_.clear(); Nit: Enclose in {}'s. http://codereview.chromium.org/8536047/diff/20001/ui/gfx/render_text_linux.h File ui/gfx/render_text_linux.h (right): http://codereview.chromium.org/8536047/diff/20001/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:42: const SelectionModel& selection_model) OVERRIDE; Nit: If you name the parameter |model|, you can keep this on one line. http://codereview.chromium.org/8536047/diff/24002/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): http://codereview.chromium.org/8536047/diff/24002/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:348: void RenderText::Draw(Canvas* canvas) { Can you make this return early if text().empty()? Then we can remove that same logic out of DrawVisualText(). http://codereview.chromium.org/8536047/diff/24002/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (left): http://codereview.chromium.org/8536047/diff/24002/ui/gfx/render_text_win.cc#o... ui/gfx/render_text_win.cc:174: void RenderTextWin::Draw(Canvas* canvas) { I have a CL that adds an EnsureLayout() call here: http://codereview.chromium.org/8633019/ Since it looks like that CL will land before this one, can you make sure RenderTextWin::Draw() will call EnsureLayout() with your CL too? You can override it the same way as you've done in RenderTextLinux. (We can probably clean this up further by making the base class call EnsureLayout() and making EnsureLayout() virtual, but I suggest to leave that to a future CL.)
http://codereview.chromium.org/8536047/diff/20001/ui/gfx/render_text.h File ui/gfx/render_text.h (left): http://codereview.chromium.org/8536047/diff/20001/ui/gfx/render_text.h#oldcode25 ui/gfx/render_text.h:25: const int kStrikeWidth = 2; On 2011/11/29 15:41:37, Alexei Svitkine wrote: > Delete this constant since its no longer used with your patch. Done. http://codereview.chromium.org/8536047/diff/20001/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/8536047/diff/20001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:196: selection_visual_bounds_.clear(); On 2011/11/29 15:41:37, Alexei Svitkine wrote: > Nit: Enclose in {}'s. Done. But is it that if the conditional body (not condition itself) is one line, "{}" is omitted? http://codereview.chromium.org/8536047/diff/20001/ui/gfx/render_text_linux.h File ui/gfx/render_text_linux.h (right): http://codereview.chromium.org/8536047/diff/20001/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:42: const SelectionModel& selection_model) OVERRIDE; On 2011/11/29 15:41:37, Alexei Svitkine wrote: > Nit: If you name the parameter |model|, you can keep this on one line. Done. http://codereview.chromium.org/8536047/diff/24002/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): http://codereview.chromium.org/8536047/diff/24002/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:348: void RenderText::Draw(Canvas* canvas) { On 2011/11/29 15:41:37, Alexei Svitkine wrote: > Can you make this return early if text().empty()? Then we can remove that same > logic out of DrawVisualText(). bypass DrawSelection and DrarVisualText when text().empty(). And removed the logic from RenderTextWin. http://codereview.chromium.org/8536047/diff/24002/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (left): http://codereview.chromium.org/8536047/diff/24002/ui/gfx/render_text_win.cc#o... ui/gfx/render_text_win.cc:174: void RenderTextWin::Draw(Canvas* canvas) { On 2011/11/29 15:41:37, Alexei Svitkine wrote: > I have a CL that adds an EnsureLayout() call here: > > http://codereview.chromium.org/8633019/ > > Since it looks like that CL will land before this one, can you make sure > RenderTextWin::Draw() will call EnsureLayout() with your CL too? > > You can override it the same way as you've done in RenderTextLinux. > > (We can probably clean this up further by making the base class call > EnsureLayout() and making EnsureLayout() virtual, but I suggest to leave that to > a future CL.) > Done. And I made EnsureLayout() to base too.
LGTM http://codereview.chromium.org/8536047/diff/20001/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/8536047/diff/20001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:196: selection_visual_bounds_.clear(); On 2011/11/29 23:07:15, xji wrote: > On 2011/11/29 15:41:37, Alexei Svitkine wrote: > > Nit: Enclose in {}'s. > > Done. > But is it that if the conditional body (not condition itself) is one line, "{}" > is omitted? I think the preference is to have {}'s unless both are each a single line. I've gotten that feedback from a past code review by Mark Mentovai, who's also on OWNERS of our style guide (http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml).
Rubber stamp LGTM
http://codereview.chromium.org/8536047/diff/25001/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/8536047/diff/25001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:204: if (from == to) This used to return a single 0-width rect, and that's the current Win behavior. This may not cause a regression, but we should still clarify expected behavior with comment at the function decl and make the cross-platform behavior consistent. http://codereview.chromium.org/8536047/diff/25001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): http://codereview.chromium.org/8536047/diff/25001/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:369: return; Just remove this.
http://codereview.chromium.org/8536047/diff/25001/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/8536047/diff/25001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:204: if (from == to) On 2011/11/30 00:20:37, msw wrote: > This used to return a single 0-width rect, and that's the current Win behavior. > This may not cause a regression, but we should still clarify expected behavior > with comment at the function decl and make the cross-platform behavior > consistent. Is there any reason that Win returns a single 0-width rect? should I change that to return an empty vector instead?
LGTM with pretty much any way you'd go on GetSelectionBounds. http://codereview.chromium.org/8536047/diff/25001/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/8536047/diff/25001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:204: if (from == to) On 2011/11/30 00:45:09, xji wrote: > On 2011/11/30 00:20:37, msw wrote: > > This used to return a single 0-width rect, and that's the current Win > behavior. > > This may not cause a regression, but we should still clarify expected behavior > > with comment at the function decl and make the cross-platform behavior > > consistent. > > Is there any reason that Win returns a single 0-width rect? should I change that > to return an empty vector instead? It looks fine to change Win to return an empty vector. There are only a couple callsites of GetSubstringBounds; all look robust against empty vectors and essentially no-op on empty rects. Remember to add a comment either way at decl. It looks like most/all callsites are using GetSubstringBounds to get the selection bounds; perhaps it's worth consolidating the selection bounds caching logic if RenderTextWin::Draw is called constantly like RenderTextLinux::Draw. It might even make sense to deprecate this signature and just support GetSelectionBounds.
http://codereview.chromium.org/8536047/diff/25001/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/8536047/diff/25001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:204: if (from == to) On 2011/11/30 00:54:51, msw wrote: > On 2011/11/30 00:45:09, xji wrote: > > On 2011/11/30 00:20:37, msw wrote: > > > This used to return a single 0-width rect, and that's the current Win > > behavior. > > > This may not cause a regression, but we should still clarify expected > behavior > > > with comment at the function decl and make the cross-platform behavior > > > consistent. > > > > Is there any reason that Win returns a single 0-width rect? should I change > that > > to return an empty vector instead? > > It looks fine to change Win to return an empty vector. There are only a couple > callsites of GetSubstringBounds; all look robust against empty vectors and > essentially no-op on empty rects. Remember to add a comment either way at decl. done. > > It looks like most/all callsites are using GetSubstringBounds to get the > selection bounds; perhaps it's worth consolidating the selection bounds caching > logic if RenderTextWin::Draw is called constantly like RenderTextLinux::Draw. It > might even make sense to deprecate this signature and just support > GetSelectionBounds. after this patch, GetSubstringBounds() is only called in RenderText::DrawSelection(). We used to have this as a public API, so I still keep it as not only used for get selection bounds. But you are right that it might be worth to change it. Also, I think Drawing will be called constantly in Windows too. I will leave these 2 part to future version. http://codereview.chromium.org/8536047/diff/25001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): http://codereview.chromium.org/8536047/diff/25001/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:369: return; On 2011/11/30 00:20:37, msw wrote: > Just remove this. Duh. Removed.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xji@chromium.org/8536047/25004
Change committed as 112188 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
