Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1531213002:
Mac: Implement firstRectForCharacterRange:actualRange in BridgedContentView. (Closed)
|
Created:
5 years ago by karandeepb Modified:
4 years, 11 months ago CC:
chromium-reviews, tfarina, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMac: Implement firstRectForCharacterRange:actualRange in BridgedContentView.
This CL implements firstRectForCharacterRange:actualRange in
BridgedContentView. This fixes bugs related to IME positioning when
MacViews is enabled.
BUG=462478
TEST=Enable chrome://flags/#mac-views-dialogs. Click on the
Add bookmark(Star) icon on browser address bar. Use an IME to write
text in the "Name" textfield. Verify that the IME candidate list is
positioned correctly.
Committed: https://crrev.com/c302121e33acc8115774892a65b82b41e89600cc
Cr-Commit-Position: refs/heads/master@{#368843}
Patch Set 1 #Patch Set 2 : Removed reduntant statement. #
Total comments: 36
Patch Set 3 : Handled review comments. #Patch Set 4 : Corrected comment formatting #
Total comments: 48
Patch Set 5 : Addressed review comments and made tests concise. #
Total comments: 34
Patch Set 6 : Addressed review comments. #
Total comments: 9
Patch Set 7 : Addressed review comments #
Total comments: 11
Patch Set 8 : Handled review comments #Patch Set 9 : Rebased #
Messages
Total messages: 27 (8 generated)
Description was changed from ========== Formatted code Got GetCaretBounds working and modified tests Applied patch from reitveld BUG= ========== to ========== Mac: Implement firstRectForCharacterRange:actualRange in BridgedContentView. This CL implements firstRectForCharacterRange:actualRange in BridgedContentView. This fixes bugs related to IME positioning when MacViews is enabled. BUG=462478 TEST=Enable chrome://flags/#mac-views-dialogs. Click on the Add bookmark(Star) icon on browser address bar. Use an IME to write text in the "Name" textfield. Verify that the IME candidate list is positioned correctly. ==========
karandeepb@chromium.org changed reviewers: + tapted@chromium.org
PTAL Trent. Thanks!
first pass (I've only had a quick look at the tests) https://codereview.chromium.org/1531213002/diff/20001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/1531213002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:60: const ui::TextInputClient* textInputClient_, no trailing underscores on function arguments (also it should be hacker_style since we're not between @implementation/@end). Maybe calling this `client` will neaten up the formatting a bit https://codereview.chromium.org/1531213002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:61: const gfx::Range& range, range -> requested_range? (for clarity) https://codereview.chromium.org/1531213002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:62: gfx::Range& actual_range) { chromium style doesn't allow non-const references, so this will need to be a pointer https://codereview.chromium.org/1531213002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:64: return gfx::Rect(0, 0, 0, 0); return gfx::Rect(); https://codereview.chromium.org/1531213002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:69: actual_range.set_start(range.start()); *actual_range = gfx::Range(requested_rangerange.start()); or is there a downside to doing *actual_range = requested_range? https://codereview.chromium.org/1531213002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:75: if (range.is_reversed()) I'm pretty sure it's impossible for requested_range to be reversed -- NSRange doesn't support reversed ranges since it is (index+unsignedlength) rather than (start_index,end_index] Perhaps // NSRange doesn't support reversed ranges. DCHECK(!requested_range.is_reversed()); https://codereview.chromium.org/1531213002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:82: if (!composition_range.Contains(gfx::Range(range.start())) || why not just `if (!composition_range.Contains(requested_range)`? https://codereview.chromium.org/1531213002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:95: end_index - 1 - composition_range.start(), ¤t_rect)) would RenderWidgetHostViewMac::ConvertCharacterRangeToCompositionRange(..) help isolate some of this arithmetic each time you call GetCompositionCharacterBounds()? https://codereview.chromium.org/1531213002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:99: range.start() - composition_range.start(), ¤t_rect)) this `else if` needs curlies, since it's attached to an `if` that has curlies https://codereview.chromium.org/1531213002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:105: // In browser multi-line textfields are not supported by Chrome currently. Perhaps something like // Toolkit-views textfields are always single-line, so no need to check for // line breaks. https://codereview.chromium.org/1531213002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:115: return caret_bounds; can this be dealt with outside the loop? (maybe start the loop at range.start() + 1). Perhaps initialize union_rect with the first character (return caret_bounds if that fails), then move the declaration for current_rect into the for loop. I feel like maybe you could combine that with the empty-range case as well. e.g. gfx::Rect union_rect; size_t first_index = /* some logic */ if (!client->GetCompositionCharacterBounds(first_index, &union_rect)) return caret_bounds; // If the range is empty, return a zero-width rectangle corresponding.. if (requested_range.is_empty()) { // For <case>, use the right-side of the rect instead of the left. if (range.start() == end_index) union_rect.set_origin(union_rect.top_right()); union_rect.set_width(0); return union_rect; } for (size_t i = range.start() + 1; i < range.end(); i++) { /* etc */ https://codereview.chromium.org/1531213002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:116: else { if any bit of an if/else chain has a curly, it all needs curlies https://codereview.chromium.org/1531213002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:661: gfx::Range requested_range(range); since this is between @implementation and @end (i.e. it's an ObjC area of code), we need to use camelCase for variable names (go/objcstyle) https://codereview.chromium.org/1531213002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:666: if (NSEqualRects(ns_rect, NSZeroRect)) { nit: move this up a line and do if (rect.IsEmpty()) return NSZeroRect; Also what happens if this early exit is omitted? (E.g. is the only downside that the for the error case, the popup appears in the top-left rather than the bottom-left of screen - that might be fine) https://codereview.chromium.org/1531213002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:670: // ns_rect is in screen coordinates with origin at top left. Convert to Apple return gfx::ScreenRectToNSRect(screen_rect); https://codereview.chromium.org/1531213002/diff/20001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_native_widget_unittest.mm (right): https://codereview.chromium.org/1531213002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:48: gfx::Rect ConvertCoordinateSystem(NSRect rect) { gfx::ScreenRectFromNSRect https://codereview.chromium.org/1531213002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:521: ui::TextInputClient* textInputClient = [ns_view_ textInputClient]; hacker_style (eg. `client` for brevity too) https://codereview.chromium.org/1531213002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:565: std::vector<gfx::Rect> char_bounds, caret_bounds; it's uncommon to declare multiple things on a line like this in chrome. You can set the size when constructing too, rather than using .resize
PTAL Trent. Thanks. https://codereview.chromium.org/1531213002/diff/20001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/1531213002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:60: const ui::TextInputClient* textInputClient_, On 2015/12/17 08:42:20, tapted wrote: > no trailing underscores on function arguments (also it should be hacker_style > since we're not between @implementation/@end). Maybe calling this `client` will > neaten up the formatting a bit Done. https://codereview.chromium.org/1531213002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:61: const gfx::Range& range, On 2015/12/17 08:42:20, tapted wrote: > range -> requested_range? (for clarity) Done. https://codereview.chromium.org/1531213002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:62: gfx::Range& actual_range) { On 2015/12/17 08:42:20, tapted wrote: > chromium style doesn't allow non-const references, so this will need to be a > pointer Done. https://codereview.chromium.org/1531213002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:64: return gfx::Rect(0, 0, 0, 0); On 2015/12/17 08:42:20, tapted wrote: > return gfx::Rect(); Done. https://codereview.chromium.org/1531213002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:69: actual_range.set_start(range.start()); On 2015/12/17 08:42:19, tapted wrote: > *actual_range = gfx::Range(requested_rangerange.start()); > > or is there a downside to doing > > *actual_range = requested_range? actual_range should correspond to the returned rectangle. Hence when we are returning caret_bounds, it feels more natural to return an empty range. https://codereview.chromium.org/1531213002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:75: if (range.is_reversed()) On 2015/12/17 08:42:19, tapted wrote: > I'm pretty sure it's impossible for requested_range to be reversed -- NSRange > doesn't support reversed ranges since it is (index+unsignedlength) rather than > (start_index,end_index] > > Perhaps > > // NSRange doesn't support reversed ranges. > DCHECK(!requested_range.is_reversed()); Done. https://codereview.chromium.org/1531213002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:82: if (!composition_range.Contains(gfx::Range(range.start())) || On 2015/12/17 08:42:20, tapted wrote: > why not just `if (!composition_range.Contains(requested_range)`? Done. https://codereview.chromium.org/1531213002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:95: end_index - 1 - composition_range.start(), ¤t_rect)) On 2015/12/17 08:42:19, tapted wrote: > would RenderWidgetHostViewMac::ConvertCharacterRangeToCompositionRange(..) help > isolate some of this arithmetic each time you call > GetCompositionCharacterBounds()? Done. https://codereview.chromium.org/1531213002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:99: range.start() - composition_range.start(), ¤t_rect)) On 2015/12/17 08:42:20, tapted wrote: > this `else if` needs curlies, since it's attached to an `if` that has curlies Done. https://codereview.chromium.org/1531213002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:105: // In browser multi-line textfields are not supported by Chrome currently. On 2015/12/17 08:42:19, tapted wrote: > Perhaps something like > > // Toolkit-views textfields are always single-line, so no need to check for > // line breaks. Done. https://codereview.chromium.org/1531213002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:115: return caret_bounds; On 2015/12/17 08:42:19, tapted wrote: > can this be dealt with outside the loop? (maybe start the loop at range.start() > + 1). > > Perhaps initialize union_rect with the first character (return caret_bounds if > that fails), then move the declaration for current_rect into the for loop. > > I feel like maybe you could combine that with the empty-range case as well. > > e.g. > > gfx::Rect union_rect; > size_t first_index = /* some logic */ > if (!client->GetCompositionCharacterBounds(first_index, &union_rect)) > return caret_bounds; > > // If the range is empty, return a zero-width rectangle corresponding.. > if (requested_range.is_empty()) { > // For <case>, use the right-side of the rect instead of the left. > if (range.start() == end_index) > union_rect.set_origin(union_rect.top_right()); > union_rect.set_width(0); > return union_rect; > } > > for (size_t i = range.start() + 1; i < range.end(); i++) { > /* etc */ Done. https://codereview.chromium.org/1531213002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:116: else { On 2015/12/17 08:42:19, tapted wrote: > if any bit of an if/else chain has a curly, it all needs curlies Done. https://codereview.chromium.org/1531213002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:661: gfx::Range requested_range(range); On 2015/12/17 08:42:20, tapted wrote: > since this is between @implementation and @end (i.e. it's an ObjC area of code), > we need to use camelCase for variable names (go/objcstyle) Done. https://codereview.chromium.org/1531213002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:666: if (NSEqualRects(ns_rect, NSZeroRect)) { On 2015/12/17 08:42:19, tapted wrote: > nit: move this up a line and do > > if (rect.IsEmpty()) > return NSZeroRect; > > Also what happens if this early exit is omitted? (E.g. is the only downside that > the for the error case, the popup appears in the top-left rather than the > bottom-left of screen - that might be fine) Removed the early exit. Also, its unlikely for textInputClient_ to be nil, so shouldn't matter. https://codereview.chromium.org/1531213002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:670: // ns_rect is in screen coordinates with origin at top left. Convert to Apple On 2015/12/17 08:42:20, tapted wrote: > return gfx::ScreenRectToNSRect(screen_rect); Done. https://codereview.chromium.org/1531213002/diff/20001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_native_widget_unittest.mm (right): https://codereview.chromium.org/1531213002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:48: gfx::Rect ConvertCoordinateSystem(NSRect rect) { On 2015/12/17 08:42:20, tapted wrote: > gfx::ScreenRectFromNSRect Done. https://codereview.chromium.org/1531213002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:521: ui::TextInputClient* textInputClient = [ns_view_ textInputClient]; On 2015/12/17 08:42:20, tapted wrote: > hacker_style (eg. `client` for brevity too) Done. https://codereview.chromium.org/1531213002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:565: std::vector<gfx::Rect> char_bounds, caret_bounds; On 2015/12/17 08:42:20, tapted wrote: > it's uncommon to declare multiple things on a line like this in chrome. > > You can set the size when constructing too, rather than using .resize Done.
Yeah this IME/composition range stuff is still pretty unfamiliar to me. We should get one of the ime experts like shuchen@ to take a look too https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:60: gfx::Rect GetFirstRectForRangeHelper(const ui::TextInputClient* client, This method is big enough that it should have a comment https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:78: if (!client->GetCompositionTextRange(&composition_range)) Can the composition_range be reversed? (will that break stuff?) DCHECK that it's not? https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:87: gfx::Rect union_rect; Maybe a comment about what's going on with |union_rect|. Like, // Pick a single character's bounds as the initial rect, then grow it to the full |requested_range| if possible. https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:88: size_t composition_count = composition_count -> composition_range.length() (also can it be zero? can that break stuff?) https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:90: size_t first_index = relative_range.start() == composition_count Needs a comment here - it's not immediately obvious what's happening. i.e. // Pick the initial character based on ... https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:96: // If |requested_range| is empty, return a zero width rectangle corresponding comment needs updating https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:99: if (relative_range.start() == composition_count) { Maybe this can be saved in a bool up above to help readability. like const request_is_composition_end = relative_range.start() == composition_count; (not sure if that's a good name, since I'm not familiar with this IME stuff) https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:100: // Return a rectangle corresponding to the top right corner of the last Rather than a comment just saying what the code does, it can be good to say why it's necessary, and why what it's doing works (or, sometimes, why doing the alternative is bad -- i.e. if a special case is needed and it's not clear why, the special case needs a justification). E.g. // To handle a requested range that's <something> move the rectangle to the right side to ensure the candidate window appears <somehow>. https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:102: union_rect.set_origin(union_rect.top_right()); Is top_right always correct? (e.g. is there any such thing as RTL in IME, which might require a different position?) https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:662: gfx::Range requestedRange(range); nit: temporary not needed https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:668: // Convert rect to AppKit screen coordinates. nit: comment not needed (it's on the function prototype :) https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_native_widget_unittest.mm (right): https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:512: TEST_F(BridgedNativeWidgetTest, TextInput_FirstRectForCharacterRange) { nit: brief comment summarising what's being tested https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:515: NSRange range; move declarations closer to first use. Typically in Chrome, if a variable can be assigned when declared, then that's how it's done (in a test it's fine to re-assign it later, even if that code is unrelated) https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:516: NSRange actual_range = NSMakeRange(-1, -1); NSMakeRange takes unsigned integers. An invalid range is usually NSMakeRange(NSNotFound, 0) https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:523: bounds = client->GetCaretBounds(); Can this be a constant in the test? (if it depends on font sizes and things, then this is fine, it just needs a comment). There's a bunch of interesting guidelines - this is something like 'don't put logic in tests' go/tott/2014/07/episode-338-dont-put-logic-in-tests.html i.e. something like // Start with the caret bounds. gfx::Rect bounds(0, 0, 2, 10); EXPECT_EQ(bounds, client->GetCaretBounds()); https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:525: EXPECT_TRUE(gfx::ScreenRectFromNSRect(rect) == bounds); EXPECT_EQ should work for gfx::Bounds, and give better error messages. More below. You can also use EXPECT_NSEQ for NSRects https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:526: EXPECT_TRUE(NSEqualRanges(NSMakeRange(0, 0), actual_range)); EXPECT_EQ_RANGE. More below https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:532: It might be good to split up the test here into separate cases. E.g. a separate method for the empty cases like TEST_F(BridgedNativeWidgetTest, TextInput_FirstRectForCharacterRange_Empty) TEST_F(BridgedNativeWidgetTest, TextInput_FirstRectForCharacterRange) https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:533: // Set composition with caret before second character('e'). nit: space before open paren https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:536: composition.text = base::UTF8ToUTF16("test_str"); nit: ASCIIToUTF16 is what usually gets used for string literals https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:537: size_t count = composition.text.length(); `count` is pretty vague here. It also breaks the logic-in-tests thing. Maybe const size_t kTextLength = 8; https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:549: EXPECT_FALSE(gfx::ScreenRectFromNSRect(rect) == bounds); comment about this? (what's it testing, and why is it false? can you test against the rect that it *should* be equal to?) https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:574: // Query outside composition range. These all need a bit more commentary. I.e. say why this "Query outside composition range" is different to the one before. https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:580: std::vector<gfx::Rect> char_bounds(count); It's unusual to see exhaustive testing like this: if a test fails, it's much harder to see what needs fixing. Is this exercising any new code paths? Can you use a test string of just two characters and "unroll" all the loops manually to get coverage of all the cases?
PTAL Trent. I have updated the tests to make them more concise and clearer. Have also updated the return values in case of unusual cases. https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:60: gfx::Rect GetFirstRectForRangeHelper(const ui::TextInputClient* client, On 2015/12/22 02:33:57, tapted wrote: > This method is big enough that it should have a comment Done. https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:78: if (!client->GetCompositionTextRange(&composition_range)) On 2015/12/22 02:33:58, tapted wrote: > Can the composition_range be reversed? (will that break stuff?) DCHECK that it's > not? Its not the case for a textfield atleast - https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/controls/.... Added a DCHECK. https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:87: gfx::Rect union_rect; On 2015/12/22 02:33:58, tapted wrote: > Maybe a comment about what's going on with |union_rect|. Like, // Pick a single > character's bounds as the initial rect, then grow it to the full > |requested_range| if possible. Done. https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:88: size_t composition_count = On 2015/12/22 02:33:58, tapted wrote: > composition_count -> composition_range.length() (also can it be zero? can that > break stuff?) Done. It can't be zero since we have already checked for the case where there is no composition text in - if (!client->HasCompositionText()) ... https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:90: size_t first_index = relative_range.start() == composition_count On 2015/12/22 02:33:57, tapted wrote: > Needs a comment here - it's not immediately obvious what's happening. i.e. // > Pick the initial character based on ... Done. https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:96: // If |requested_range| is empty, return a zero width rectangle corresponding On 2015/12/22 02:33:58, tapted wrote: > comment needs updating Updated. I had kept this because relative_range is just for arithmetic convenience. Actual bounds returned still correspond to requested_range. https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:99: if (relative_range.start() == composition_count) { On 2015/12/22 02:33:58, tapted wrote: > Maybe this can be saved in a bool up above to help readability. like > > const request_is_composition_end = relative_range.start() == composition_count; > > (not sure if that's a good name, since I'm not familiar with this IME stuff) Done. https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:100: // Return a rectangle corresponding to the top right corner of the last On 2015/12/22 02:33:58, tapted wrote: > Rather than a comment just saying what the code does, it can be good to say why > it's necessary, and why what it's doing works (or, sometimes, why doing the > alternative is bad -- i.e. if a special case is needed and it's not clear why, > the special case needs a justification). > > E.g. // To handle a requested range that's <something> move the rectangle to the > right side to ensure the candidate window appears <somehow>. This is a case which I can't reproduce in practice, hence can't really explain how this affects the candidate window. Returning the rectangle to the right of the last compositioned character feels natural here. https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:102: union_rect.set_origin(union_rect.top_right()); On 2015/12/22 02:33:57, tapted wrote: > Is top_right always correct? (e.g. is there any such thing as RTL in IME, which > might require a different position?) I couldn't find any RTL IME. I had also talked to Matt regarding this, and he was also not aware of any RTL IME. The only case which might be worth looking at is using IMEs in an RTL context(when the system language is RTL). Apple's documentation for firstRectForCharacterRange:actualRange: says that the returned rectangle can have a negative size if text flows to left. However, I couldn't observe any visible difference on setting the returned rectangle's size to be negative. https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:662: gfx::Range requestedRange(range); On 2015/12/22 02:33:57, tapted wrote: > nit: temporary not needed Done. https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:668: // Convert rect to AppKit screen coordinates. On 2015/12/22 02:33:58, tapted wrote: > nit: comment not needed (it's on the function prototype :) Done. https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_native_widget_unittest.mm (right): https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:512: TEST_F(BridgedNativeWidgetTest, TextInput_FirstRectForCharacterRange) { On 2015/12/22 02:33:58, tapted wrote: > nit: brief comment summarising what's being tested Done. https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:515: NSRange range; On 2015/12/22 02:33:58, tapted wrote: > move declarations closer to first use. Typically in Chrome, if a variable can be > assigned when declared, then that's how it's done (in a test it's fine to > re-assign it later, even if that code is unrelated) Done. https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:516: NSRange actual_range = NSMakeRange(-1, -1); On 2015/12/22 02:33:58, tapted wrote: > NSMakeRange takes unsigned integers. An invalid range is usually > NSMakeRange(NSNotFound, 0) Done. https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:523: bounds = client->GetCaretBounds(); On 2015/12/22 02:33:58, tapted wrote: > Can this be a constant in the test? (if it depends on font sizes and things, > then this is fine, it just needs a comment). There's a bunch of interesting > guidelines - this is something like 'don't put logic in tests' > go/tott/2014/07/episode-338-dont-put-logic-in-tests.html > > i.e. something like > > // Start with the caret bounds. > gfx::Rect bounds(0, 0, 2, 10); > EXPECT_EQ(bounds, client->GetCaretBounds()); Not quite sure how to make this a constant, since we are checking that the actual returned bounds from firstRectForCharacterRange correspond to the expected caret bounds, when the caret is placed at the appropriate position. I have separated this logic into a separate function GetCaretBoundsForPosition(..) to make it clearer. https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:525: EXPECT_TRUE(gfx::ScreenRectFromNSRect(rect) == bounds); On 2015/12/22 02:33:58, tapted wrote: > EXPECT_EQ should work for gfx::Bounds, and give better error messages. More > below. > > You can also use EXPECT_NSEQ for NSRects Done. https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:526: EXPECT_TRUE(NSEqualRanges(NSMakeRange(0, 0), actual_range)); On 2015/12/22 02:33:58, tapted wrote: > EXPECT_EQ_RANGE. More below Done. https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:532: On 2015/12/22 02:33:58, tapted wrote: > It might be good to split up the test here into separate cases. E.g. a separate > method for the empty cases like > > TEST_F(BridgedNativeWidgetTest, TextInput_FirstRectForCharacterRange_Empty) > > TEST_F(BridgedNativeWidgetTest, TextInput_FirstRectForCharacterRange) Done. https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:533: // Set composition with caret before second character('e'). On 2015/12/22 02:33:58, tapted wrote: > nit: space before open paren Done. https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:536: composition.text = base::UTF8ToUTF16("test_str"); On 2015/12/22 02:33:58, tapted wrote: > nit: ASCIIToUTF16 is what usually gets used for string literals Done. https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:537: size_t count = composition.text.length(); On 2015/12/22 02:33:58, tapted wrote: > `count` is pretty vague here. It also breaks the logic-in-tests thing. Maybe > > const size_t kTextLength = 8; Done. https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:549: EXPECT_FALSE(gfx::ScreenRectFromNSRect(rect) == bounds); On 2015/12/22 02:33:58, tapted wrote: > comment about this? (what's it testing, and why is it false? can you test > against the rect that it *should* be equal to?) Done. https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:574: // Query outside composition range. On 2015/12/22 02:33:58, tapted wrote: > These all need a bit more commentary. I.e. say why this "Query outside > composition range" is different to the one before. Done. https://codereview.chromium.org/1531213002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:580: std::vector<gfx::Rect> char_bounds(count); On 2015/12/22 02:33:58, tapted wrote: > It's unusual to see exhaustive testing like this: if a test fails, it's much > harder to see what needs fixing. Is this exercising any new code paths? > > Can you use a test string of just two characters and "unroll" all the loops > manually to get coverage of all the cases? That would turn out to be a bit messy, since we will still need to compute individual character bounds from caret bounds, the logic for which seems more concise when written using loops. Similar exhaustive testing was done in render_widget_host_view_mac_unittest.mm- https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
karandeepb@chromium.org changed reviewers: + shuchen@chromium.org
shuchen@ - Can you please look at this CL since we are not that familiar with IME.
https://codereview.chromium.org/1531213002/diff/80001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/1531213002/diff/80001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:60: // Helper function for firstRectForCharacterRange:actualRange defined below. This comment doesn't add much beyond the function name. It should be something like "Query |client| for the screen rectangle encompassing |requested_range|. |actual_range| is set to the subset of the requested range that can be matched by |client|. If none of the range fits, returns the bounds of the input caret." Maybe more than that if there are other special behaviours you've implemented. You can also cite another function if this is "Standard" behaviour documented elsewhere that's just being implemented here for mac. https://codereview.chromium.org/1531213002/diff/80001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:69: gfx::Rect default_rect = gfx::Rect(); nit: no need for the ` = gfx::Rect()` - just use default constructor https://codereview.chromium.org/1531213002/diff/80001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:96: gfx::Range relative_range(requested_range.start() - composition_range.start(), nit: declare const https://codereview.chromium.org/1531213002/diff/80001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:102: size_t composition_count = composition_range.length(); nit: declare const https://codereview.chromium.org/1531213002/diff/80001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:105: // use the bounds for the last compositioned character. Is using the last character bounds better than using the caret bounds in this case? (or is that done by something else already?) shuchen might know the right answer :) https://codereview.chromium.org/1531213002/diff/80001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:131: gfx::Rect current_rect; move declaration into loop body https://codereview.chromium.org/1531213002/diff/80001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_native_widget_unittest.mm (right): https://codereview.chromium.org/1531213002/diff/80001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:20: #include "ui/gfx/mac/coordinate_conversion.h" nit: import https://codereview.chromium.org/1531213002/diff/80001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:52: NSRange* caret_range = nil) { I don't think anything relies on the default argument. Remove? https://codereview.chromium.org/1531213002/diff/80001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:73: NSRange* caret_range = nil) { I think there's one call that wants the default - maybe just omit it. default arguments haven't really taken off in Chrome yet, since they've only recently been blessed by go/cppguide https://codereview.chromium.org/1531213002/diff/80001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:561: const base::string16 kTestString = base::ASCIIToUTF16("test_str"); So there's a weird convention (or, the literal interpretation of the style guide) for naming constants that only primitive types are named as kFooBar (e.g. char[] or int is fine, but std::string should be `const std::string test_string;`) https://codereview.chromium.org/1531213002/diff/80001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:565: // Test empty ranges within the composition range. nit: follow comment with the expectations, E.g. "Test empty range after the second character: should return caret bounds (since..?)" https://codereview.chromium.org/1531213002/diff/80001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:573: query_range = NSMakeRange(kTextLength, 0); likewise, "Test empty range at the end of the text: should return <..>" https://codereview.chromium.org/1531213002/diff/80001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:591: // Make sure not crashing by passing NULL pointer instead of actualRange. nit: NULL->null https://codereview.chromium.org/1531213002/diff/80001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:592: rect = [ns_view_ firstRectForCharacterRange:query_range actualRange:NULL]; nit: NULL->nullptr (again it's convention: `null` in comments, but always `nullptr` in code) https://codereview.chromium.org/1531213002/diff/80001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:600: const base::string16 kTestString = base::ASCIIToUTF16("test_str"); Is there a minimum test string that can give code coverage without requiring the loops below? It's usually confusing to put EXPECT_* statements into loops, since you're not sure what the input data is. You can add SCOPED_TRACE(testing::Message() << "Range [begin, end) = [" << i << ", " << j << ")"); to the inner loop, but it's clumsy. No loops would be best.
PTAL Trent. Thanks. Also, let me know if I should test this change with any particular IME. Currently, I have tested this with Mac's native IMEs and Google Japanese IME for Mac. https://codereview.chromium.org/1531213002/diff/80001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/1531213002/diff/80001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:60: // Helper function for firstRectForCharacterRange:actualRange defined below. On 2015/12/30 06:48:52, tapted wrote: > This comment doesn't add much beyond the function name. It should be something > like "Query |client| for the screen rectangle encompassing |requested_range|. > |actual_range| is set to the subset of the requested range that can be matched > by |client|. If none of the range fits, returns the bounds of the input caret." > > Maybe more than that if there are other special behaviours you've implemented. > You can also cite another function if this is "Standard" behaviour documented > elsewhere that's just being implemented here for mac. Done. https://codereview.chromium.org/1531213002/diff/80001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:69: gfx::Rect default_rect = gfx::Rect(); On 2015/12/30 06:48:52, tapted wrote: > nit: no need for the ` = gfx::Rect()` - just use default constructor Done. https://codereview.chromium.org/1531213002/diff/80001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:96: gfx::Range relative_range(requested_range.start() - composition_range.start(), On 2015/12/30 06:48:52, tapted wrote: > nit: declare const Done. https://codereview.chromium.org/1531213002/diff/80001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:102: size_t composition_count = composition_range.length(); On 2015/12/30 06:48:52, tapted wrote: > nit: declare const Done. https://codereview.chromium.org/1531213002/diff/80001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:105: // use the bounds for the last compositioned character. On 2015/12/30 06:48:52, tapted wrote: > Is using the last character bounds better than using the caret bounds in this > case? (or is that done by something else already?) shuchen might know the right > answer :) This is how RenderWidgetHostViewMac::GetCachedFirstRectForCharacterRange(...) handles this - https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re.... Also, I think it's fairly intuitive. Since the query range is [composition_count, composition_count], we are just returning an empty rectangle to the right of the last compositioned character(i.e. character at index composition_count-1). https://codereview.chromium.org/1531213002/diff/80001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:131: gfx::Rect current_rect; On 2015/12/30 06:48:52, tapted wrote: > move declaration into loop body Done. https://codereview.chromium.org/1531213002/diff/80001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_native_widget_unittest.mm (right): https://codereview.chromium.org/1531213002/diff/80001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:20: #include "ui/gfx/mac/coordinate_conversion.h" On 2015/12/30 06:48:53, tapted wrote: > nit: import Done. https://codereview.chromium.org/1531213002/diff/80001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:52: NSRange* caret_range = nil) { On 2015/12/30 06:48:53, tapted wrote: > I don't think anything relies on the default argument. Remove? Done. https://codereview.chromium.org/1531213002/diff/80001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:73: NSRange* caret_range = nil) { On 2015/12/30 06:48:52, tapted wrote: > I think there's one call that wants the default - maybe just omit it. default > arguments haven't really taken off in Chrome yet, since they've only recently > been blessed by go/cppguide Done. https://codereview.chromium.org/1531213002/diff/80001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:561: const base::string16 kTestString = base::ASCIIToUTF16("test_str"); On 2015/12/30 06:48:53, tapted wrote: > So there's a weird convention (or, the literal interpretation of the style > guide) for naming constants that only primitive types are named as kFooBar (e.g. > char[] or int is fine, but std::string should be `const std::string > test_string;`) Can't find this in the cpp style guide - https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#.... Also, similar naming has been followed at other places in the file, should I refactor it? https://codereview.chromium.org/1531213002/diff/80001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:565: // Test empty ranges within the composition range. On 2015/12/30 06:48:53, tapted wrote: > nit: follow comment with the expectations, E.g. > > "Test empty range after the second character: should return caret bounds > (since..?)" Done. https://codereview.chromium.org/1531213002/diff/80001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:573: query_range = NSMakeRange(kTextLength, 0); On 2015/12/30 06:48:53, tapted wrote: > likewise, "Test empty range at the end of the text: should return <..>" Done. https://codereview.chromium.org/1531213002/diff/80001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:591: // Make sure not crashing by passing NULL pointer instead of actualRange. On 2015/12/30 06:48:52, tapted wrote: > nit: NULL->null Done. https://codereview.chromium.org/1531213002/diff/80001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:592: rect = [ns_view_ firstRectForCharacterRange:query_range actualRange:NULL]; On 2015/12/30 06:48:53, tapted wrote: > nit: NULL->nullptr > > (again it's convention: `null` in comments, but always `nullptr` in code) Done. https://codereview.chromium.org/1531213002/diff/80001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:600: const base::string16 kTestString = base::ASCIIToUTF16("test_str"); On 2015/12/30 06:48:53, tapted wrote: > Is there a minimum test string that can give code coverage without requiring the > loops below? > > It's usually confusing to put EXPECT_* statements into loops, since you're not > sure what the input data is. > > You can add SCOPED_TRACE(testing::Message() << "Range [begin, end) = [" << i << > ", " << j << ")"); to the inner loop, but it's clumsy. No loops would be best. Removed the loops. Somehow, this approach did not strike me earlier! https://codereview.chromium.org/1531213002/diff/100001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/1531213002/diff/100001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:122: if (relative_range.is_empty()) { Can any reviewer confirm whether the handling of this case is correct? Apple documentation says to return a zero width rectangle coinciding with the caret position for this case - https://developer.apple.com/library/mac/documentation/Cocoa/Reference/NSTextI...: . However, in RenderWidgetHostViewMac::GetCachedFirstRectForCharacterRange(...), the case is handled as below. In practice, I can't get this code path to execute since Mac's native IMEs only query with an empty range when there is no composition text(which seems reasonable), and hence we return the caret bounds for it(line 89). Also, in my tests Google's Japanese IME for mac never queries with an empty range.
lgtm % nits. However, we should wait for an IME expert to weigh in before committing. https://codereview.chromium.org/1531213002/diff/80001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_native_widget_unittest.mm (right): https://codereview.chromium.org/1531213002/diff/80001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:561: const base::string16 kTestString = base::ASCIIToUTF16("test_str"); On 2015/12/31 02:42:44, karandeepb wrote: > On 2015/12/30 06:48:53, tapted wrote: > > So there's a weird convention (or, the literal interpretation of the style > > guide) for naming constants that only primitive types are named as kFooBar > (e.g. > > char[] or int is fine, but std::string should be `const std::string > > test_string;`) > > Can't find this in the cpp style guide - > https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#.... > Also, similar naming has been followed at other places in the file, should I > refactor it? It says "Variables declared constexpr or const, and whose value is fixed for the duration of the program". Things with constructors only take on a value when they're constructed and that value is gone when it's destroyed, so I've had senior reviewers ping me on this in the past. So it's something to be aware of, but it's still a "guide" :). You can leave this as kTestString if you prefer - consistency is important too, and it can sometimes trump other style concerns. I'll leave it up to you. (Anecdotally, there's a `kTestString.size()` elsewhere in the file -- this looks really jarring to me, so I'd support changing them all, but it should be in a separate CL since there's a lot). https://codereview.chromium.org/1531213002/diff/80001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:592: rect = [ns_view_ firstRectForCharacterRange:query_range actualRange:NULL]; On 2015/12/31 02:42:43, karandeepb wrote: > On 2015/12/30 06:48:53, tapted wrote: > > nit: NULL->nullptr > > > > (again it's convention: `null` in comments, but always `nullptr` in code) > > Done. (ooh, I should say -- and you may have already figured this one out -- 'nil' is also acceptable in an objective C context if it's a pointer to a [subclass of] NSObject*. Here nullptr is more correct since NSRange is a C-style struct and not a subtype of NSObject.) https://codereview.chromium.org/1531213002/diff/100001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/1531213002/diff/100001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:19: #include "ui/gfx/mac/coordinate_conversion.h" nit: import https://codereview.chromium.org/1531213002/diff/100001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:122: if (relative_range.is_empty()) { On 2015/12/31 02:42:44, karandeepb wrote: > Can any reviewer confirm whether the handling of this case is correct? Apple > documentation says to return a zero width rectangle coinciding with the caret > position for this case - > https://developer.apple.com/library/mac/documentation/Cocoa/Reference/NSTextI...: > . However, in RenderWidgetHostViewMac::GetCachedFirstRectForCharacterRange(...), > the case is handled as below. In practice, I can't get this code path to execute > since Mac's native IMEs only query with an empty range when there is no > composition text(which seems reasonable), and hence we return the caret bounds > for it(line 89). Also, in my tests Google's Japanese IME for mac never queries > with an empty range. I don't know the answer from a NSTextInputClient standpoint, but there's typically a strong aversion to committing (non-test) code that's effectively dead in a Chrome release (i.e. impossible to execute, or only executed in tests). In this case, you can spelunk through the code review where it originally went in: (assuming I followed it correctly), the logic came from here: https://chromiumcodereview.appspot.com/10656019/diff2/17010:21006/content/bro... The reviewer gave the comment, "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() ?" It won't be the case that IME on Mac will always be Apple's implementation (Chrome has support for plugins/extensions as well), so I think it's important to keep the logic. It would be great if you could single out the test case that gives coverage of this block though -- annotate the EXPECT_FOO call with a comment along with a quote from the apple documentation that says our implementation might be non-standard. (This way, if the test starts failing for someone who later tries to align it to the standard, they can make sense of why it is the way it is). https://codereview.chromium.org/1531213002/diff/100001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_native_widget_unittest.mm (right): https://codereview.chromium.org/1531213002/diff/100001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget_unittest.mm:620: // Set composition text. nit: this comment doesn't add much https://codereview.chromium.org/1531213002/diff/100001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget_unittest.mm:623: SetCompositionText(client, kTestString, 1, nil); so here, `nullptr` is slightly more correct than `nil`. (I think that the degree to which reviewers really care is diminishing though, so there's a good chance nothing would be said about it). I guess it's useful to consider something like NSString* y = nil; NSRange* x = nil; printf("%lu", y.length); // prints 0 printf("%lu", x->length); // crashes So, using "nil" could make readers think about it differently, e.g. think some operations are safe when they're not.
Patchset #7 (id:120001) has been deleted
https://codereview.chromium.org/1531213002/diff/80001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_native_widget_unittest.mm (right): https://codereview.chromium.org/1531213002/diff/80001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:561: const base::string16 kTestString = base::ASCIIToUTF16("test_str"); On 2016/01/04 02:28:10, tapted wrote: > On 2015/12/31 02:42:44, karandeepb wrote: > > On 2015/12/30 06:48:53, tapted wrote: > > > So there's a weird convention (or, the literal interpretation of the style > > > guide) for naming constants that only primitive types are named as kFooBar > > (e.g. > > > char[] or int is fine, but std::string should be `const std::string > > > test_string;`) > > > > Can't find this in the cpp style guide - > > > https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#.... > > Also, similar naming has been followed at other places in the file, should I > > refactor it? > > It says "Variables declared constexpr or const, and whose value is fixed for the > duration of the program". Things with constructors only take on a value when > they're constructed and that value is gone when it's destroyed, so I've had > senior reviewers ping me on this in the past. > > So it's something to be aware of, but it's still a "guide" :). You can leave > this as kTestString if you prefer - consistency is important too, and it can > sometimes trump other style concerns. I'll leave it up to you. (Anecdotally, > there's a `kTestString.size()` elsewhere in the file -- this looks really > jarring to me, so I'd support changing them all, but it should be in a separate > CL since there's a lot). Done. Will refactor the other usages in a separate CL. https://codereview.chromium.org/1531213002/diff/80001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:592: rect = [ns_view_ firstRectForCharacterRange:query_range actualRange:NULL]; On 2016/01/04 02:28:10, tapted wrote: > On 2015/12/31 02:42:43, karandeepb wrote: > > On 2015/12/30 06:48:53, tapted wrote: > > > nit: NULL->nullptr > > > > > > (again it's convention: `null` in comments, but always `nullptr` in code) > > > > Done. > > (ooh, I should say -- and you may have already figured this one out -- 'nil' is > also acceptable in an objective C context if it's a pointer to a [subclass of] > NSObject*. Here nullptr is more correct since NSRange is a C-style struct and > not a subtype of NSObject.) Didn't know all of it. Thanks! https://codereview.chromium.org/1531213002/diff/100001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/1531213002/diff/100001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:19: #include "ui/gfx/mac/coordinate_conversion.h" On 2016/01/04 02:28:10, tapted wrote: > nit: import Done. https://codereview.chromium.org/1531213002/diff/100001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:122: if (relative_range.is_empty()) { On 2016/01/04 02:28:10, tapted wrote: > On 2015/12/31 02:42:44, karandeepb wrote: > > Can any reviewer confirm whether the handling of this case is correct? Apple > > documentation says to return a zero width rectangle coinciding with the caret > > position for this case - > > > https://developer.apple.com/library/mac/documentation/Cocoa/Reference/NSTextI...: > > . However, in > RenderWidgetHostViewMac::GetCachedFirstRectForCharacterRange(...), > > the case is handled as below. In practice, I can't get this code path to > execute > > since Mac's native IMEs only query with an empty range when there is no > > composition text(which seems reasonable), and hence we return the caret bounds > > for it(line 89). Also, in my tests Google's Japanese IME for mac never queries > > with an empty range. > > I don't know the answer from a NSTextInputClient standpoint, but there's > typically a strong aversion to committing (non-test) code that's effectively > dead in a Chrome release (i.e. impossible to execute, or only executed in > tests). > > In this case, you can spelunk through the code review where it originally went > in: > > (assuming I followed it correctly), the logic came from here: > > https://chromiumcodereview.appspot.com/10656019/diff2/17010:21006/content/bro... > > The reviewer gave the comment, "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() ?" > > It won't be the case that IME on Mac will always be Apple's implementation > (Chrome has support for plugins/extensions as well), so I think it's important > to keep the logic. It would be great if you could single out the test case that > gives coverage of this block though -- annotate the EXPECT_FOO call with a > comment along with a quote from the apple documentation that says our > implementation might be non-standard. (This way, if the test starts failing for > someone who later tries to align it to the standard, they can make sense of why > it is the way it is). Done. https://codereview.chromium.org/1531213002/diff/100001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_native_widget_unittest.mm (right): https://codereview.chromium.org/1531213002/diff/100001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget_unittest.mm:620: // Set composition text. On 2016/01/04 02:28:10, tapted wrote: > nit: this comment doesn't add much Done. https://codereview.chromium.org/1531213002/diff/100001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget_unittest.mm:623: SetCompositionText(client, kTestString, 1, nil); On 2016/01/04 02:28:10, tapted wrote: > so here, `nullptr` is slightly more correct than `nil`. (I think that the degree > to which reviewers really care is diminishing though, so there's a good chance > nothing would be said about it). > > I guess it's useful to consider something like > > NSString* y = nil; > NSRange* x = nil; > printf("%lu", y.length); // prints 0 > printf("%lu", x->length); // crashes > > So, using "nil" could make readers think about it differently, e.g. think some > operations are safe when they're not. Done.
Sorry for late review. I just came back from vacation and swamped by high priority stuff. I may need one or two days before getting back to this cl. Sorry~
On 2016/01/06 08:55:41, Shu Chen wrote: > Sorry for late review. I just came back from vacation and swamped by high > priority stuff. I may need one or two days before getting back to this cl. > Sorry~ No worries. Thanks!
https://codereview.chromium.org/1531213002/diff/140001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/1531213002/diff/140001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:80: default_rect.set_width(0); Why zero? https://codereview.chromium.org/1531213002/diff/140001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:90: return default_rect; Should return the selection bounds (if valid) I think. https://codereview.chromium.org/1531213002/diff/140001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:127: union_rect.set_origin(union_rect.top_right()); top_right() may not be correct for RTL text. https://codereview.chromium.org/1531213002/diff/140001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:150: return union_rect; (optional) can you please make the code conciser? e.g. replace line 102~150: size_t from = requested_range.start() - composition_range.start(); size_t to = requested_range.end() - composition_range.start(); gfx::Rect union_rect; if (from == composition_range.length()) { if (!client->GetCompositionCharacterBounds(from - 1, &union_rect)) return default_rect; union_rect.set_origin(union_rect.top_right()); union_rect.set_width(0); } else { for (size_t i = from; i < to; i++) { gfx::Rect current_rect; if (!client->GetCompositionCharacterBounds(i, ¤t_rect)) { *actual_range = ...; return union_rect; } union_rect.Union(current_rect); } } *actual_range = requested_range; return union_rect;
PTAL Shu Chen. Thanks. https://codereview.chromium.org/1531213002/diff/140001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/1531213002/diff/140001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:80: default_rect.set_width(0); On 2016/01/08 04:18:53, Shu Chen wrote: > Why zero? From what I have seen, IME clients query with an empty range when there is no composition text. The documentation at https://developer.apple.com/library/mac/documentation/Cocoa/Reference/NSTextI...: says to return a 0 width rectangle coinciding with the insertion point when the width of requested range is 0. Hence I thought it to be a good default value. https://codereview.chromium.org/1531213002/diff/140001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:90: return default_rect; On 2016/01/08 04:18:53, Shu Chen wrote: > Should return the selection bounds (if valid) I think. Do you mean the bounds corresponding to the 'whole' area of selected text or the caret bounds with the width not set to 0? The spec of GetCaretBounds() in textInputClient says that it returns the selection bounds if there is a selection. However, in practice, it returns the caret bounds(and not the bounds corresponding to the full selection). https://codereview.chromium.org/1531213002/diff/140001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:127: union_rect.set_origin(union_rect.top_right()); On 2016/01/08 04:18:53, Shu Chen wrote: > top_right() may not be correct for RTL text. Done. https://codereview.chromium.org/1531213002/diff/140001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:150: return union_rect; On 2016/01/08 04:18:53, Shu Chen wrote: > (optional) can you please make the code conciser? e.g. replace line 102~150: > > size_t from = requested_range.start() - composition_range.start(); > size_t to = requested_range.end() - composition_range.start(); > gfx::Rect union_rect; > if (from == composition_range.length()) { > if (!client->GetCompositionCharacterBounds(from - 1, &union_rect)) > return default_rect; > union_rect.set_origin(union_rect.top_right()); > union_rect.set_width(0); > } else { > for (size_t i = from; i < to; i++) { > gfx::Rect current_rect; > if (!client->GetCompositionCharacterBounds(i, ¤t_rect)) { > *actual_range = ...; > return union_rect; > } > union_rect.Union(current_rect); > } > } > *actual_range = requested_range; > return union_rect; This does not handle the case where from==to but from is inside the composition range. This will return the empty rectangle but the current code returns a 0 width rectangle corresponding to from. Taking care of it, should result in code similar to the current one. I have tried to make the code a bit concise. Apart from that, most of the space is occupied by comments. Let me know if any of them seem redundant to you.
https://codereview.chromium.org/1531213002/diff/140001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/1531213002/diff/140001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:80: default_rect.set_width(0); On 2016/01/12 05:43:52, karandeepb wrote: > On 2016/01/08 04:18:53, Shu Chen wrote: > > Why zero? > > From what I have seen, IME clients query with an empty range when there is no > composition text. The documentation at > https://developer.apple.com/library/mac/documentation/Cocoa/Reference/NSTextI...: > says to return a 0 width rectangle coinciding with the insertion point when the > width of requested range is 0. Hence I thought it to be a good default value. Acknowledged. https://codereview.chromium.org/1531213002/diff/140001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:90: return default_rect; On 2016/01/12 05:43:52, karandeepb wrote: > On 2016/01/08 04:18:53, Shu Chen wrote: > > Should return the selection bounds (if valid) I think. > > Do you mean the bounds corresponding to the 'whole' area of selected text or the > caret bounds with the width not set to 0? The spec of GetCaretBounds() in > textInputClient says that it returns the selection bounds if there is a > selection. However, in practice, it returns the caret bounds(and not the bounds > corresponding to the full selection). Ah, thanks for the clarification. That should be a separated bug to fix. https://codereview.chromium.org/1531213002/diff/140001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:150: return union_rect; On 2016/01/12 05:43:52, karandeepb wrote: > On 2016/01/08 04:18:53, Shu Chen wrote: > > (optional) can you please make the code conciser? e.g. replace line 102~150: > > > > size_t from = requested_range.start() - composition_range.start(); > > size_t to = requested_range.end() - composition_range.start(); > > gfx::Rect union_rect; > > if (from == composition_range.length()) { > > if (!client->GetCompositionCharacterBounds(from - 1, &union_rect)) > > return default_rect; > > union_rect.set_origin(union_rect.top_right()); > > union_rect.set_width(0); > > } else { > > for (size_t i = from; i < to; i++) { > > gfx::Rect current_rect; > > if (!client->GetCompositionCharacterBounds(i, ¤t_rect)) { > > *actual_range = ...; > > return union_rect; > > } > > union_rect.Union(current_rect); > > } > > } > > *actual_range = requested_range; > > return union_rect; > > This does not handle the case where from==to but from is inside the composition > range. This will return the empty rectangle but the current code returns a 0 > width rectangle corresponding to from. Taking care of it, should result in code > similar to the current one. I have tried to make the code a bit concise. Apart > from that, most of the space is occupied by comments. Let me know if any of them > seem redundant to you. Acknowledged.
lgtm
The CQ bit was checked by karandeepb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org, shuchen@chromium.org Link to the patchset: https://codereview.chromium.org/1531213002/#ps180001 (title: "Rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1531213002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1531213002/180001
Message was sent while issue was closed.
Description was changed from ========== Mac: Implement firstRectForCharacterRange:actualRange in BridgedContentView. This CL implements firstRectForCharacterRange:actualRange in BridgedContentView. This fixes bugs related to IME positioning when MacViews is enabled. BUG=462478 TEST=Enable chrome://flags/#mac-views-dialogs. Click on the Add bookmark(Star) icon on browser address bar. Use an IME to write text in the "Name" textfield. Verify that the IME candidate list is positioned correctly. ========== to ========== Mac: Implement firstRectForCharacterRange:actualRange in BridgedContentView. This CL implements firstRectForCharacterRange:actualRange in BridgedContentView. This fixes bugs related to IME positioning when MacViews is enabled. BUG=462478 TEST=Enable chrome://flags/#mac-views-dialogs. Click on the Add bookmark(Star) icon on browser address bar. Use an IME to write text in the "Name" textfield. Verify that the IME candidate list is positioned correctly. ==========
Message was sent while issue was closed.
Committed patchset #9 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Mac: Implement firstRectForCharacterRange:actualRange in BridgedContentView. This CL implements firstRectForCharacterRange:actualRange in BridgedContentView. This fixes bugs related to IME positioning when MacViews is enabled. BUG=462478 TEST=Enable chrome://flags/#mac-views-dialogs. Click on the Add bookmark(Star) icon on browser address bar. Use an IME to write text in the "Name" textfield. Verify that the IME candidate list is positioned correctly. ========== to ========== Mac: Implement firstRectForCharacterRange:actualRange in BridgedContentView. This CL implements firstRectForCharacterRange:actualRange in BridgedContentView. This fixes bugs related to IME positioning when MacViews is enabled. BUG=462478 TEST=Enable chrome://flags/#mac-views-dialogs. Click on the Add bookmark(Star) icon on browser address bar. Use an IME to write text in the "Name" textfield. Verify that the IME candidate list is positioned correctly. Committed: https://crrev.com/c302121e33acc8115774892a65b82b41e89600cc Cr-Commit-Position: refs/heads/master@{#368843} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/c302121e33acc8115774892a65b82b41e89600cc Cr-Commit-Position: refs/heads/master@{#368843} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
