Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2348143003:
MacViews: Implement Force Touch/Mac dictionary lookup for Textfields. (Closed)
|
Created:
4 years, 3 months ago by karandeepb Modified:
4 years, 2 months ago CC:
chromium-reviews, tfarina, James Su, shuchen+watch_chromium.org, yusukes+watch_chromium.org, nona+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMacViews: Implement Force Touch/Mac dictionary lookup for Textfields.
This CL adds support to MacViews for Force touch and dictionary lookups. This is
achieved by implementing quickLookWithEvent: on the BridgedContentView. A new
interface called WordLookupClient is added which is to be implemented by views
which support word lookups. Currently only views::Textfield implements this
interface. A virtual method GetWordLookupClient is added to views::View to
retrieve the WordLookupClient associated with a given View. Support is added to
the RenderText class to retreive the word displayed at a given point along with
its styling information. A DecoratedText class is also introduced to encapsulate
the styling information for some given text.
Note, this CL only adds support for dictionary lookups to Views::Textfields.
Doc- https://docs.google.com/a/google.com/document/d/17QYrMpIXpk-FpGanjhJ-Yx33njNQy7B5oL_bxb7-vns/edit?usp=sharing.
BUG=640502
TEST= Enable chrome://flags/#mac-views-native-dialogs. Open Bookmark Bubble.
Enter some text in the name textfield. Hover over the text and press
Ctrl+Command+D. Verify that a dictionary popup appears showing the definition of
the word, the mouse was hovering on.
Committed: https://crrev.com/a56ce7b0a7e774caebf3a4dd774d6d5d7d9b8d2a
Cr-Commit-Position: refs/heads/master@{#423419}
Patch Set 1 : --- #Patch Set 2 : --- #
Total comments: 24
Patch Set 3 : Address review. #
Total comments: 3
Patch Set 4 #Patch Set 5 : Fix compile. #
Total comments: 20
Patch Set 6 : Rebase. #Patch Set 7 : Address review. #
Total comments: 2
Patch Set 8 : Address review. #
Total comments: 2
Patch Set 9 : Rebase. #
Total comments: 1
Patch Set 10 : Add tests. #Patch Set 11 : Fix tests. #Patch Set 12 : Typos. #Patch Set 13 : Fix compile. #
Total comments: 35
Patch Set 14 : Address msw's review comments. #Patch Set 15 : Add LookupWordClient interface. #Patch Set 16 : Typos, cleanup, comments etc. #Patch Set 17 : Views export, reduntant forward decl. #
Total comments: 8
Patch Set 18 : Address nits. Rename LookupWordClient->WordLookupClient. #Patch Set 19 : Call EnsureLayout in GetDecoratedTextForRange. #
Total comments: 26
Patch Set 20 : Address nits. #
Messages
Total messages: 109 (77 generated)
Description was changed from ========== -- BUG= ========== to ========== -- BUG= ==========
The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
Patchset #6 (id:100001) has been deleted
Patchset #5 (id:80001) has been deleted
Description was changed from ========== -- BUG= ========== to ========== This CL adds supports to MacViews for Force touch and dictionary popups. This is achieved by implement quickLookWithEvent: on the BridgedContentView. A new method called GetWordAtPoint is added to views::View to retreive the styled string at a particular point. Currently only Views::Textfield implements this method. Support for Views::Labels will follow. RenderText implementations are also modified to support retreiving the word at a given point. A new typedef NativeStyledString is introduced to denote the styled string for different platforms. Currently, it has a meaningful value only on Mac. BUG=640502 ==========
Patchset #1 (id:1) has been deleted
karandeepb@chromium.org changed reviewers: + tapted@chromium.org
PTAL Trent. WDYT of the approach/design? Will add tests/cleanup CL description subsequently.
just did a quick skim.. https://codereview.chromium.org/2348143003/diff/140001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/2348143003/diff/140001/ui/gfx/render_text.h#n... ui/gfx/render_text.h:733: // Expands |range| to it's nearest word boundaries and returns the resulting nit: it's -> its https://codereview.chromium.org/2348143003/diff/140001/ui/gfx/render_text.h#n... ui/gfx/render_text.h:739: virtual bool GetStyledStringForRange(const Range& range, this should be in protected: - private virtuals don't really make sense https://codereview.chromium.org/2348143003/diff/140001/ui/gfx/render_text_har... File ui/gfx/render_text_harfbuzz.h (right): https://codereview.chromium.org/2348143003/diff/140001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.h:268: bool GetStyledStringForRange(const Range& range, move to protected https://codereview.chromium.org/2348143003/diff/140001/ui/gfx/render_text_har... File ui/gfx/render_text_harfbuzz_mac.mm (right): https://codereview.chromium.org/2348143003/diff/140001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz_mac.mm:33: NSMutableAttributedString* str) const { I'm not sure I like the idea of plumbing through an NSMutableAttributedString :/. WDYT about borrowing some ideas from content/common/mac/attributed_string_coder.h E.g. rather than taking an NSMutableAttributedString*, take a std:vector<DecoratedText>* struct DecoratedText { struct RangedAttribute { gfx::Range range; gfx::Font font; // is there more? Seems it's just [DIAGONAL_]STRIKE that's missing, which probably isn't important for this. // Disallow default construction of gfx::Font, since that's slow. RangedAttribute() = delete; /* add constructor that works */ }; base::string16 string; std::vector<RangedAttribute> attributes; }; Then this stuff should be able to be implemented somewhere more generic, like render_text.cc https://codereview.chromium.org/2348143003/diff/140001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz_mac.mm:86: [str applyFontTraits:NSItalicFontMask range:strRange]; does this work for bold+italic (i.e. should the traits be OR'd together?) https://codereview.chromium.org/2348143003/diff/140001/ui/gfx/render_text_mac.mm File ui/gfx/render_text_mac.mm (right): https://codereview.chromium.org/2348143003/diff/140001/ui/gfx/render_text_mac... ui/gfx/render_text_mac.mm:22: #include "third_party/skia/include/ports/SkTypeface_mac.h" remove? https://codereview.chromium.org/2348143003/diff/140001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2348143003/diff/140001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:748: const gfx::Point& locationInContent = ui::EventLocationFromNative(theEvent); no reference (I know C++ allows this to extend the lifetime of an rvalue, but it doesn't get used much in chrome) https://codereview.chromium.org/2348143003/diff/140001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:756: NSMutableAttributedString* word = Use scoped_nsobject - autorelease has additional overheads https://codereview.chromium.org/2348143003/diff/140001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:760: target->GetWordAtPoint(locationInTarget, word, &baselinePoint); If there's an existing selection, does quickLook look that up instead of just the word? https://codereview.chromium.org/2348143003/diff/140001/ui/views/controls/text... File ui/views/controls/textfield/textfield.h (right): https://codereview.chromium.org/2348143003/diff/140001/ui/views/controls/text... ui/views/controls/textfield/textfield.h:210: bool GetWordAtPoint(const gfx::Point& point, maybe GetStyledWordAtPoint
https://codereview.chromium.org/2348143003/diff/140001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2348143003/diff/140001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:760: target->GetWordAtPoint(locationInTarget, word, &baselinePoint); On 2016/09/21 04:06:56, tapted wrote: > If there's an existing selection, does quickLook look that up instead of just > the word? Ah, I checked this, and it doesn't :). But you can look up a selection via the context menu - we need to anticipate support for this later on. https://codereview.chromium.org/2348143003/diff/140001/ui/views/view.h File ui/views/view.h (right): https://codereview.chromium.org/2348143003/diff/140001/ui/views/view.h#newcod... ui/views/view.h:580: virtual bool GetWordAtPoint(const gfx::Point& point, ooh - also I don't think we can justify adding this to the View interface (particularly if we need another to get this for a selection). TextInputClient - maybe. Or it may even be sensible to do some RTTI on Textfield::kViewClassName and a static cast. Or it could actually be OK - this is something to check with sky@, but we'll need a stronger justification. E.g. it's kinda like ToolTips, but currently very mac specific - did you explore alternatives already? (Or can you think of alternatives? (possibly bad ones))
https://codereview.chromium.org/2348143003/diff/140001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2348143003/diff/140001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:760: target->GetWordAtPoint(locationInTarget, word, &baselinePoint); On 2016/09/21 04:44:14, tapted wrote: > On 2016/09/21 04:06:56, tapted wrote: > > If there's an existing selection, does quickLook look that up instead of just > > the word? > > Ah, I checked this, and it doesn't :). But you can look up a selection via the > context menu - we need to anticipate support for this later on. Yeah I had thought about that. The private RenderText::GetStyledStringForRange method could be used for it later on. https://codereview.chromium.org/2348143003/diff/140001/ui/views/view.h File ui/views/view.h (right): https://codereview.chromium.org/2348143003/diff/140001/ui/views/view.h#newcod... ui/views/view.h:580: virtual bool GetWordAtPoint(const gfx::Point& point, On 2016/09/21 04:44:14, tapted wrote: > ooh - also I don't think we can justify adding this to the View interface > (particularly if we need another to get this for a selection). > > TextInputClient - maybe. > > Or it may even be sensible to do some RTTI on Textfield::kViewClassName and a > static cast. > > Or it could actually be OK - this is something to check with sky@, but we'll > need a stronger justification. E.g. it's kinda like ToolTips, but currently very > mac specific - did you explore alternatives already? (Or can you think of > alternatives? (possibly bad ones)) Yeah it seems similar to tooltips but is very Mac specific. So we need to support 2 usecases -> 1) For a given point, get the styled word and the baseline point. The point may not lie inside the focused view. 2) For a given range (selection), get the styled word and the baseline point. The range will probably always lie within the focused view (We need to think about how focus behaves with a Label with an active selection.) We need to support this for both Labels and Textfields. Some ideas, 1. Add a GetTextInputClient() method to views::View and add methods GetWordAtPoint and GetWordForRange to the TextInputClient. But then Labels will have to implement the TextInputClient interface subsequently (which feels slightly wrong since Labels never accept input). However once Labels start supporting selection, we may need it anyway. More specifically, what does the interface with Cocoa for use case 2 above look like. When we right click with an active selection, it shows a LookUp "selected text" context menu. So it will probably query the BCV for the selected Text and hence View::Labels might need to inherit from TextInputClient anyway. 2) Subsequently, we'll be adding support for selection to Views::Labels. The plan is probably to move the selection logic from a Views::Textfield to a seperate class which can be used by both Textfields and Labels. The new class will need access to the underlying render text instance and probably have a virtual GetRenderText method. How about a new class called RenderTextHostView from which both Labels and Textfields inherit and which itself inherits from Views::View. Then we can add a GetRenderTextHost() method to views::View and methods GetWordAtPoint and GetWordForRange to the RenderTextHostView. But I think the current design is fine for now. We'll have a clearer understanding of the design needed once we start supporting dictionary popups and selection for Views::Labels. Thoughts?
Patchset #6 (id:160001) has been deleted
https://codereview.chromium.org/2348143003/diff/140001/ui/views/view.h File ui/views/view.h (right): https://codereview.chromium.org/2348143003/diff/140001/ui/views/view.h#newcod... ui/views/view.h:580: virtual bool GetWordAtPoint(const gfx::Point& point, On 2016/09/21 05:37:12, karandeepb wrote: > On 2016/09/21 04:44:14, tapted wrote: > > ooh - also I don't think we can justify adding this to the View interface > > (particularly if we need another to get this for a selection). > > > > TextInputClient - maybe. > > > > Or it may even be sensible to do some RTTI on Textfield::kViewClassName and a > > static cast. > > > > Or it could actually be OK - this is something to check with sky@, but we'll > > need a stronger justification. E.g. it's kinda like ToolTips, but currently > very > > mac specific - did you explore alternatives already? (Or can you think of > > alternatives? (possibly bad ones)) > > Yeah it seems similar to tooltips but is very Mac specific. So we need to > support 2 usecases -> > > 1) For a given point, get the styled word and the baseline point. The point may > not lie inside the focused view. > 2) For a given range (selection), get the styled word and the baseline point. > The range will probably always lie within the focused view (We need to think > about how focus behaves with a Label with an active selection.) > > We need to support this for both Labels and Textfields. > > Some ideas, > 1. Add a GetTextInputClient() method to views::View and add methods > GetWordAtPoint and GetWordForRange to the TextInputClient. But then Labels will > have to implement the TextInputClient interface subsequently (which feels > slightly wrong since Labels never accept input). > > However once Labels start supporting selection, we may need it anyway. More > specifically, what does the interface with Cocoa for use case 2 above look like. > When we right click with an active selection, it shows a LookUp "selected text" > context menu. So it will probably query the BCV for the selected Text and hence > View::Labels might need to inherit from TextInputClient anyway. > > 2) Subsequently, we'll be adding support for selection to Views::Labels. The > plan is probably to move the selection logic from a Views::Textfield to a > seperate class which can be used by both Textfields and Labels. The new class > will need access to the underlying render text instance and probably have a > virtual GetRenderText method. > > How about a new class called RenderTextHostView from which both Labels and > Textfields inherit and which itself inherits from Views::View. > > Then we can add a GetRenderTextHost() method to views::View and methods > GetWordAtPoint and GetWordForRange to the RenderTextHostView. > > But I think the current design is fine for now. We'll have a clearer > understanding of the design needed once we start supporting dictionary popups > and selection for Views::Labels. Thoughts? What about putting a GetRenderText() directly on views::View? That seems like a nice abstraction - if a View notionally represents some formatted text that's displayed on screen it can return something, otherwise not. For now we can assume that (a) there is at most one RenderText per View, and (b) the RenderText is drawn at the origin of that View's internal coordinate system (or based off GetInsets()). So the signature could be something like virtual const gfx::RenderText* View::GetRenderText() { return nullptr; } Later on we might need to pass in/out a gfx::Point to handle hit tests (e.g. TableView?), or support views with the RenderText offset (icons?).
Patchset #6 (id:180001) has been deleted
Patchset #6 (id:200001) has been deleted
Patchset #6 (id:220001) has been deleted
The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL Trent. https://codereview.chromium.org/2348143003/diff/140001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/2348143003/diff/140001/ui/gfx/render_text.h#n... ui/gfx/render_text.h:733: // Expands |range| to it's nearest word boundaries and returns the resulting On 2016/09/21 04:06:55, tapted wrote: > nit: it's -> its Done. https://codereview.chromium.org/2348143003/diff/140001/ui/gfx/render_text.h#n... ui/gfx/render_text.h:739: virtual bool GetStyledStringForRange(const Range& range, On 2016/09/21 04:06:55, tapted wrote: > this should be in protected: - private virtuals don't really make sense Shouldn't private virtual functions be preferred, when the function does not need access to the base class implementation? Also as a general rule, shouldn't we try to tighten the access specifiers as much as possible? https://codereview.chromium.org/2348143003/diff/140001/ui/gfx/render_text_har... File ui/gfx/render_text_harfbuzz_mac.mm (right): https://codereview.chromium.org/2348143003/diff/140001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz_mac.mm:33: NSMutableAttributedString* str) const { On 2016/09/21 04:06:56, tapted wrote: > I'm not sure I like the idea of plumbing through an NSMutableAttributedString > :/. WDYT about borrowing some ideas from > content/common/mac/attributed_string_coder.h > > E.g. rather than taking an NSMutableAttributedString*, take a > std:vector<DecoratedText>* > > struct DecoratedText { > struct RangedAttribute { > gfx::Range range; > gfx::Font font; > // is there more? Seems it's just [DIAGONAL_]STRIKE that's missing, which > probably isn't important for this. > > // Disallow default construction of gfx::Font, since that's slow. > RangedAttribute() = delete; > /* add constructor that works */ > }; > base::string16 string; > std::vector<RangedAttribute> attributes; > }; > > > Then this stuff should be able to be implemented somewhere more generic, like > render_text.cc Yeah the typedef looked ugly. Encapsulating all the styling info. in a struct looks better. But don't think that this can be moved to render_text.cc. We need to still gather the info. from the individual runs. https://codereview.chromium.org/2348143003/diff/140001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz_mac.mm:86: [str applyFontTraits:NSItalicFontMask range:strRange]; On 2016/09/21 04:06:55, tapted wrote: > does this work for bold+italic (i.e. should the traits be OR'd together?) Thanks for the catch. Done. https://codereview.chromium.org/2348143003/diff/140001/ui/gfx/render_text_mac.mm File ui/gfx/render_text_mac.mm (right): https://codereview.chromium.org/2348143003/diff/140001/ui/gfx/render_text_mac... ui/gfx/render_text_mac.mm:22: #include "third_party/skia/include/ports/SkTypeface_mac.h" On 2016/09/21 04:06:56, tapted wrote: > remove? Removed render_text_harfbuzz_mac.mm. So this is no longer relevant. https://codereview.chromium.org/2348143003/diff/140001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2348143003/diff/140001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:748: const gfx::Point& locationInContent = ui::EventLocationFromNative(theEvent); On 2016/09/21 04:06:56, tapted wrote: > no reference (I know C++ allows this to extend the lifetime of an rvalue, but it > doesn't get used much in chrome) Done. https://codereview.chromium.org/2348143003/diff/140001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:756: NSMutableAttributedString* word = On 2016/09/21 04:06:56, tapted wrote: > Use scoped_nsobject - autorelease has additional overheads I guess it's preferred to return an autoreleased NSAttributedString from GetAttributedString() rather than a scoped_nsobject? https://codereview.chromium.org/2348143003/diff/140001/ui/views/controls/text... File ui/views/controls/textfield/textfield.h (right): https://codereview.chromium.org/2348143003/diff/140001/ui/views/controls/text... ui/views/controls/textfield/textfield.h:210: bool GetWordAtPoint(const gfx::Point& point, On 2016/09/21 04:06:56, tapted wrote: > maybe GetStyledWordAtPoint Changed to GetDecoratedWordAtPoint. https://codereview.chromium.org/2348143003/diff/140001/ui/views/view.h File ui/views/view.h (right): https://codereview.chromium.org/2348143003/diff/140001/ui/views/view.h#newcod... ui/views/view.h:580: virtual bool GetWordAtPoint(const gfx::Point& point, On 2016/09/22 00:50:03, tapted wrote: > On 2016/09/21 05:37:12, karandeepb wrote: > > On 2016/09/21 04:44:14, tapted wrote: > > > ooh - also I don't think we can justify adding this to the View interface > > > (particularly if we need another to get this for a selection). > > > > > > TextInputClient - maybe. > > > > > > Or it may even be sensible to do some RTTI on Textfield::kViewClassName and > a > > > static cast. > > > > > > Or it could actually be OK - this is something to check with sky@, but we'll > > > need a stronger justification. E.g. it's kinda like ToolTips, but currently > > very > > > mac specific - did you explore alternatives already? (Or can you think of > > > alternatives? (possibly bad ones)) > > > > Yeah it seems similar to tooltips but is very Mac specific. So we need to > > support 2 usecases -> > > > > 1) For a given point, get the styled word and the baseline point. The point > may > > not lie inside the focused view. > > 2) For a given range (selection), get the styled word and the baseline point. > > The range will probably always lie within the focused view (We need to think > > about how focus behaves with a Label with an active selection.) > > > > We need to support this for both Labels and Textfields. > > > > Some ideas, > > 1. Add a GetTextInputClient() method to views::View and add methods > > GetWordAtPoint and GetWordForRange to the TextInputClient. But then Labels > will > > have to implement the TextInputClient interface subsequently (which feels > > slightly wrong since Labels never accept input). > > > > However once Labels start supporting selection, we may need it anyway. More > > specifically, what does the interface with Cocoa for use case 2 above look > like. > > When we right click with an active selection, it shows a LookUp "selected > text" > > context menu. So it will probably query the BCV for the selected Text and > hence > > View::Labels might need to inherit from TextInputClient anyway. > > > > 2) Subsequently, we'll be adding support for selection to Views::Labels. The > > plan is probably to move the selection logic from a Views::Textfield to a > > seperate class which can be used by both Textfields and Labels. The new class > > will need access to the underlying render text instance and probably have a > > virtual GetRenderText method. > > > > How about a new class called RenderTextHostView from which both Labels and > > Textfields inherit and which itself inherits from Views::View. > > > > Then we can add a GetRenderTextHost() method to views::View and methods > > GetWordAtPoint and GetWordForRange to the RenderTextHostView. > > > > But I think the current design is fine for now. We'll have a clearer > > understanding of the design needed once we start supporting dictionary popups > > and selection for Views::Labels. Thoughts? > > > What about putting a GetRenderText() directly on views::View? That seems like a > nice abstraction - if a View notionally represents some formatted text that's > displayed on screen it can return something, otherwise not. For now we can > assume that (a) there is at most one RenderText per View, and (b) the RenderText > is drawn at the origin of that View's internal coordinate system (or based off > GetInsets()). So the signature could be something like > > virtual const gfx::RenderText* View::GetRenderText() { return nullptr; } > > Later on we might need to pass in/out a gfx::Point to handle hit tests (e.g. > TableView?), or support views with the RenderText offset (icons?). We also need a method which returns non const RenderText since we need to call non-const methods on it. Ideally the RenderText class should have used mutable for some member variables. https://codereview.chromium.org/2348143003/diff/240001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/2348143003/diff/240001/ui/gfx/render_text.cc#... ui/gfx/render_text.cc:1033: const std::vector<Rect> word_bounds = GetSubstringBounds(word_range); Had to change the approach here and use GetSubstringBounds instead of GetCursorBounds. Reason is that for bidi text, can't find a way to determine the index of the leftmost glyph. https://codereview.chromium.org/2348143003/diff/240001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2348143003/diff/240001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:233: if (attribute.underline) { FYI: Cocoa seems to ignore the underline and strike attributes while showing the popup. But I think we should still keep it to make the function generic. https://codereview.chromium.org/2348143003/diff/240001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:810: views::View::ConvertPointToTarget(hostedView_, target, &locationInTarget); Saw https://codereview.chromium.org/2148893003. So basically hostedView_->GetTooltipHandlerForPoint can return a view not in the hierarchy of the hostedView_. If this is the case, I think the documentation of View::GetTooltipHandlerForPoint should be updated since it seems to suggest that a descendent will be returned. Can this also happen for GetEventHandlerForPoint or is the ConvertPointToTarget call here safe?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Thinking about it a bit more, I don't think exposing a non const RenderText in this way on a views::View is ideal. If the owners don't like adding a GetWordAtPoint function on a views::View, I like the ideas in c#22 more, or changing RenderText to use mutable so that we can expose a const RenderText. I'll probably put together a doc, and ask the owners for their comments as well.
Description was changed from ========== This CL adds supports to MacViews for Force touch and dictionary popups. This is achieved by implement quickLookWithEvent: on the BridgedContentView. A new method called GetWordAtPoint is added to views::View to retreive the styled string at a particular point. Currently only Views::Textfield implements this method. Support for Views::Labels will follow. RenderText implementations are also modified to support retreiving the word at a given point. A new typedef NativeStyledString is introduced to denote the styled string for different platforms. Currently, it has a meaningful value only on Mac. BUG=640502 ========== to ========== This CL adds supports to MacViews for Force touch and dictionary popups. This is achieved by implement quickLookWithEvent: on the BridgedContentView. Methods are added to views::View to retreive the underlying RenderText instance. Support is added to the RenderText class to retreive the word displayed at a given point along with its' styling information. A class DecoratedText is introduced to encapsulate the styling information for some given text. Note, this CL only adds support for dictionary popups to Views::Textfields. Doc - https://docs.google.com/a/google.com/document/d/17QYrMpIXpk-FpGanjhJ-Yx33njNQ.... BUG=640502 ==========
Description was changed from ========== This CL adds supports to MacViews for Force touch and dictionary popups. This is achieved by implement quickLookWithEvent: on the BridgedContentView. Methods are added to views::View to retreive the underlying RenderText instance. Support is added to the RenderText class to retreive the word displayed at a given point along with its' styling information. A class DecoratedText is introduced to encapsulate the styling information for some given text. Note, this CL only adds support for dictionary popups to Views::Textfields. Doc - https://docs.google.com/a/google.com/document/d/17QYrMpIXpk-FpGanjhJ-Yx33njNQ.... BUG=640502 ========== to ========== This CL adds supports to MacViews for Force touch and dictionary popups. This is achieved by implement quickLookWithEvent: on the BridgedContentView. Methods are added to views::View to retreive the underlying RenderText instance. Support is added to the RenderText class to retreive the word displayed at a given point along with its' styling information. A class DecoratedText is introduced to encapsulate the styling information for some given text. Note, this CL only adds support for dictionary popups to Views::Textfields. Doc- https://docs.google.com/a/google.com/document/d/17QYrMpIXpk-FpGanjhJ-Yx33njNQ.... BUG=640502 ==========
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
https://codereview.chromium.org/2348143003/diff/280001/ui/gfx/decorated_text.h File ui/gfx/decorated_text.h (right): https://codereview.chromium.org/2348143003/diff/280001/ui/gfx/decorated_text.... ui/gfx/decorated_text.h:29: Font::Weight weight; can weight, underline, italic be retrieved from the gfx::Font? (and if so, can strike be included in the gfx::Font::FontStyle? -- there's a TODO to merge them - it's confusing that they are separate) https://codereview.chromium.org/2348143003/diff/280001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/2348143003/diff/280001/ui/gfx/render_text.cc#... ui/gfx/render_text.cc:1016: // FindCursorPosition doesn't currently support multiline. this will need a bug reference. We'll need this for lookup in selectable multiline labels :/ https://codereview.chromium.org/2348143003/diff/280001/ui/gfx/render_text.cc#... ui/gfx/render_text.cc:1020: const SelectionModel model_at_point = FindCursorPosition(point); Perhaps a note here, that FindCursorPosition calls EnsureLayout https://codereview.chromium.org/2348143003/diff/280001/ui/gfx/render_text.cc#... ui/gfx/render_text.cc:1035: return false; I think |decorated_word| should be unmodified if `false` is returned. But under what case can this happen? (Comment? Can it DCHECK instead?) https://codereview.chromium.org/2348143003/diff/280001/ui/gfx/render_text.cc#... ui/gfx/render_text.cc:1037: const Rect* left_rect = &word_bounds[0]; nit: comment why this is necessary https://codereview.chromium.org/2348143003/diff/280001/ui/gfx/render_text_har... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/2348143003/diff/280001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:1603: for (size_t i = 0; i < run_list->size(); i++) { What would be necessary to put this on the RenderText interface? E.g. RenderTextHarfbuzz has a (TextRunList wrapping a) TextRunHarfBuzz, which is quite similar to a RenderTextMac::TextRun. Hypothetically... TextRunHarfBuzz and RenderTextMac::TextRun could inherit from the same base class, and RenderText could require classes inheriting from it to provide a std::vector<TextRunBase*>. But also DecoratedText kinda feels like it should be an "input" to the TextRuns - not something that's built up from the TextRuns (maybe RenderText should have a DecoratedText member that it passes to the concrete classes. Perhaps even RenderText::ApplyColor(..) RenderText::ApplyBaselineStyle(..) RenderText::ApplyStyle(..) RenderText::ApplyWeight(..) should all be replaced by RenderText::ApplyDecorations(..) This would be a huge gain to performance, since each ApplyFoo first validates its range, then invalidates its style (requiring a full layout for subsequent range validations). https://codereview.chromium.org/2348143003/diff/280001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2348143003/diff/280001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:215: NSMutableAttributedString* str = [[NSMutableAttributedString alloc] init]; Use scoped_nsobject (and str.autorelease() at the end of the method) - this makes the method robust to someone adding an early exit later on without introducing a memory leak https://codereview.chromium.org/2348143003/diff/280001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:222: DCHECK(attribute.range.end() <= [str length]); Is DCHECK_LE a thing? https://codereview.chromium.org/2348143003/diff/280001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:227: [attrs setObject:attribute.font.GetNativeFont() Does the Objc2.0 map syntax work -- attrs[NSFontAttributeName] = .... But there's no special syntax for removing elements -- Ithink we should just start with an empty attributes map each iteration - or just a helper function to convert a decorated text entry into a NSDictionary of atrtibutes.
The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #6 (id:300001) has been deleted
PTAL Trent. https://codereview.chromium.org/2348143003/diff/280001/ui/gfx/decorated_text.h File ui/gfx/decorated_text.h (right): https://codereview.chromium.org/2348143003/diff/280001/ui/gfx/decorated_text.... ui/gfx/decorated_text.h:29: Font::Weight weight; On 2016/09/26 02:02:31, tapted wrote: > can weight, underline, italic be retrieved from the gfx::Font? I am retrieving these from TextRunHarfBuzz. Its font object does not store the correct styles, which are stored separately. Have modified RenderTextHarfBuzz::GetDecoratedTextForRange to use DeriveFont to get the Font object with the correct styles. > (and if so, can strike be included in the gfx::Font::FontStyle? -- there's a > TODO to merge them - it's confusing that they are separate) Can look at this subsequently, since it may also require changes to the underlying PlatformFont* classes as well. https://codereview.chromium.org/2348143003/diff/280001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/2348143003/diff/280001/ui/gfx/render_text.cc#... ui/gfx/render_text.cc:1016: // FindCursorPosition doesn't currently support multiline. On 2016/09/26 02:02:31, tapted wrote: > this will need a bug reference. We'll need this for lookup in selectable > multiline labels :/ Done. https://codereview.chromium.org/2348143003/diff/280001/ui/gfx/render_text.cc#... ui/gfx/render_text.cc:1020: const SelectionModel model_at_point = FindCursorPosition(point); On 2016/09/26 02:02:31, tapted wrote: > Perhaps a note here, that FindCursorPosition calls EnsureLayout Done. https://codereview.chromium.org/2348143003/diff/280001/ui/gfx/render_text.cc#... ui/gfx/render_text.cc:1035: return false; On 2016/09/26 02:02:31, tapted wrote: > I think |decorated_word| should be unmodified if `false` is returned. But under > what case can this happen? (Comment? Can it DCHECK instead?) Moved this above so decorated_word is not modified. This may happen if this method is invoked on a RenderTextMac instance. Although currently no codepath will call this on a RTM instance. However, since this method is exposed as part of the public API, I think it should handle that case. https://codereview.chromium.org/2348143003/diff/280001/ui/gfx/render_text.cc#... ui/gfx/render_text.cc:1037: const Rect* left_rect = &word_bounds[0]; On 2016/09/26 02:02:31, tapted wrote: > nit: comment why this is necessary Done. https://codereview.chromium.org/2348143003/diff/280001/ui/gfx/render_text_har... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/2348143003/diff/280001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:1603: for (size_t i = 0; i < run_list->size(); i++) { >E.g. RenderTextHarfbuzz has a (TextRunList wrapping a) TextRunHarfBuzz, which is >quite similar to a RenderTextMac::TextRun. >Hypothetically... TextRunHarfBuzz and RenderTextMac::TextRun could inherit from >the same base class, and RenderText could require classes inheriting from it to >provide a std::vector<TextRunBase*>. Yeah this seems plausible and may allow us to move more code from RTM/RTHB to the RenderText class. However, I guess this is orthogonal to this CL and can be explored subsequently. >But also DecoratedText kinda feels like it should be an "input" to the TextRuns >- not something that's built up from the TextRuns (maybe RenderText should have >a DecoratedText member that it passes to the concrete classes. Currently RenderText uses the BreakList class to store the style info which is then provided as an input to the concrete classes to create the text runs. I think the BreakList class is optimized for this use case. Also, I don't see how DecoratedText can act as an input to the concrete classes. It consists of non-overlapping ranges which have the same styles/font. The font can only be determined by the concrete classes. Also, if we were to eliminate the font, I don't see what benefits it offers over the current implementation. >This would be a huge gain to performance, since each ApplyFoo first validates >its range, then invalidates its style (requiring a full layout for subsequent >range validations). I think this is only a problem with the ApplyStyle call. You are probably referring to your comment here - https://codereview.chromium.org/1953133002/#msg8. As you mentioned, it's probably because of the comment "Do not change styles mid-grapheme to avoid breaking ligatures." at https://cs.chromium.org/chromium/src/ui/gfx/render_text.cc?cl=GROK&gsn=IsVali.... But I am not sure, if validating the range is necessary. To me, this feels something that should be handled while creating the text runs. The code which creates the run ensures that a single run has the same style. Can a single grapheme be split across multiple runs? Even if it is necessary, it can probably be handled by maintaining a temporary bool flag and temporary breaklist for the various styles, which can be bound checked when really necessary, say in EnsureLayout. But I don't see how DecoratedText helps with any of this. https://codereview.chromium.org/2348143003/diff/280001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2348143003/diff/280001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:215: NSMutableAttributedString* str = [[NSMutableAttributedString alloc] init]; On 2016/09/26 02:02:31, tapted wrote: > Use scoped_nsobject (and str.autorelease() at the end of the method) - this > makes the method robust to someone adding an early exit later on without > introducing a memory leak Done. https://codereview.chromium.org/2348143003/diff/280001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:222: DCHECK(attribute.range.end() <= [str length]); On 2016/09/26 02:02:31, tapted wrote: > Is DCHECK_LE a thing? Done. https://codereview.chromium.org/2348143003/diff/280001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:227: [attrs setObject:attribute.font.GetNativeFont() On 2016/09/26 02:02:31, tapted wrote: > Does the Objc2.0 map syntax work -- attrs[NSFontAttributeName] = .... But > there's no special syntax for removing elements -- Ithink we should just start > with an empty attributes map each iteration - or just a helper function to > convert a decorated text entry into a NSDictionary of atrtibutes. Done. The map syntax causes compiler warnings though.
looks good - I'll probably be adding ObjC array subscripting support in https://codereview.chromium.org/2193153002/ - something similar should work for ObjC dictionaries. Let's ask owners about the API - I like the approach here. It'll need some tests before landing, too. https://codereview.chromium.org/2348143003/diff/280001/ui/gfx/render_text_har... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/2348143003/diff/280001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:1603: for (size_t i = 0; i < run_list->size(); i++) { On 2016/09/26 07:00:08, karandeepb wrote: > >E.g. RenderTextHarfbuzz has a (TextRunList wrapping a) TextRunHarfBuzz, which > is > >quite similar to a RenderTextMac::TextRun. > >Hypothetically... TextRunHarfBuzz and RenderTextMac::TextRun could inherit from > >the same base class, and RenderText could require classes inheriting from it to > >provide a std::vector<TextRunBase*>. > > Yeah this seems plausible and may allow us to move more code from RTM/RTHB to > the RenderText class. However, I guess this is orthogonal to this CL and can be > explored subsequently. > > >But also DecoratedText kinda feels like it should be an "input" to the TextRuns > >- not something that's built up from the TextRuns (maybe RenderText should have > >a DecoratedText member that it passes to the concrete classes. > > Currently RenderText uses the BreakList class to store the style info which is > then provided as an input to the concrete classes to create the text runs. I > think the BreakList class is optimized for this use case. > Also, I don't see how DecoratedText can act as an input to the concrete classes. > It consists of non-overlapping ranges which have the same styles/font. The font > can only be determined by the concrete classes. Also, if we were to eliminate > the font, I don't see what benefits it offers over the current implementation. > > >This would be a huge gain to performance, since each ApplyFoo first validates > >its range, then invalidates its style (requiring a full layout for subsequent > >range validations). > > I think this is only a problem with the ApplyStyle call. You are probably > referring to your comment here - > https://codereview.chromium.org/1953133002/#msg8. As you mentioned, it's > probably because of the comment "Do not change styles mid-grapheme to avoid > breaking ligatures." at > https://cs.chromium.org/chromium/src/ui/gfx/render_text.cc?cl=GROK&gsn=IsVali.... > But I am not sure, if validating the range is necessary. To me, this feels > something that should be handled while creating the text runs. The code which > creates the run ensures that a single run has the same style. Can a single > grapheme be split across multiple runs? > > Even if it is necessary, it can probably be handled by maintaining a temporary > bool flag and temporary breaklist for the various styles, which can be bound > checked when really necessary, say in EnsureLayout. > > But I don't see how DecoratedText helps with any of this. DecoratedText could allow someone to apply multiple/different styles to multiple ranges at once. The NSAtributedString beginEditing / endEditing probably exists for a similar reason. https://codereview.chromium.org/2348143003/diff/340001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2348143003/diff/340001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:249: return [str.release() autorelease]; return str.autorelease();
The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2348143003/diff/280001/ui/gfx/render_text_har... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/2348143003/diff/280001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:1603: for (size_t i = 0; i < run_list->size(); i++) { On 2016/09/26 08:07:26, tapted wrote: > On 2016/09/26 07:00:08, karandeepb wrote: > > >E.g. RenderTextHarfbuzz has a (TextRunList wrapping a) TextRunHarfBuzz, which > > is > > >quite similar to a RenderTextMac::TextRun. > > >Hypothetically... TextRunHarfBuzz and RenderTextMac::TextRun could inherit > from > > >the same base class, and RenderText could require classes inheriting from it > to > > >provide a std::vector<TextRunBase*>. > > > > Yeah this seems plausible and may allow us to move more code from RTM/RTHB to > > the RenderText class. However, I guess this is orthogonal to this CL and can > be > > explored subsequently. > > > > >But also DecoratedText kinda feels like it should be an "input" to the > TextRuns > > >- not something that's built up from the TextRuns (maybe RenderText should > have > > >a DecoratedText member that it passes to the concrete classes. > > > > Currently RenderText uses the BreakList class to store the style info which is > > then provided as an input to the concrete classes to create the text runs. I > > think the BreakList class is optimized for this use case. > > Also, I don't see how DecoratedText can act as an input to the concrete > classes. > > It consists of non-overlapping ranges which have the same styles/font. The > font > > can only be determined by the concrete classes. Also, if we were to eliminate > > the font, I don't see what benefits it offers over the current implementation. > > > > > >This would be a huge gain to performance, since each ApplyFoo first validates > > >its range, then invalidates its style (requiring a full layout for subsequent > > >range validations). > > > > I think this is only a problem with the ApplyStyle call. You are probably > > referring to your comment here - > > https://codereview.chromium.org/1953133002/#msg8. As you mentioned, it's > > probably because of the comment "Do not change styles mid-grapheme to avoid > > breaking ligatures." at > > > https://cs.chromium.org/chromium/src/ui/gfx/render_text.cc?cl=GROK&gsn=IsVali.... > > But I am not sure, if validating the range is necessary. To me, this feels > > something that should be handled while creating the text runs. The code which > > creates the run ensures that a single run has the same style. Can a single > > grapheme be split across multiple runs? > > > > Even if it is necessary, it can probably be handled by maintaining a temporary > > bool flag and temporary breaklist for the various styles, which can be bound > > checked when really necessary, say in EnsureLayout. > > > > But I don't see how DecoratedText helps with any of this. > > DecoratedText could allow someone to apply multiple/different styles to multiple > ranges at once. The NSAtributedString beginEditing / endEditing probably exists > for a similar reason. Oh ok, so basically providing a way for RenderText clients to change multiple styles for multiple ranges simultaneously is what you were referring to, with each ApplyDecoration call probably triggering a relayout, which in turn may also reduce the state we have to maintain for style invalidation. https://codereview.chromium.org/2348143003/diff/340001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2348143003/diff/340001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:249: return [str.release() autorelease]; On 2016/09/26 08:07:26, tapted wrote: > return str.autorelease(); Done.
karandeepb@chromium.org changed reviewers: + msw@chromium.org, sky@chromium.org
PTAL msw@ for ui/gfx/*, ui/views/controls/textfield/* and chrome/* review. PTAL sky@ for ui/views/view.* review. Please see the doc linked in the CL description for more context. I still need to think about what tests to add. Also, please see whether the changes to view.h look acceptable.
Description was changed from ========== This CL adds supports to MacViews for Force touch and dictionary popups. This is achieved by implement quickLookWithEvent: on the BridgedContentView. Methods are added to views::View to retreive the underlying RenderText instance. Support is added to the RenderText class to retreive the word displayed at a given point along with its' styling information. A class DecoratedText is introduced to encapsulate the styling information for some given text. Note, this CL only adds support for dictionary popups to Views::Textfields. Doc- https://docs.google.com/a/google.com/document/d/17QYrMpIXpk-FpGanjhJ-Yx33njNQ.... BUG=640502 ========== to ========== This CL adds supports to MacViews for Force touch and dictionary popups. This is achieved by implement quickLookWithEvent: on the BridgedContentView. Methods are added to views::View to retreive the underlying RenderText instance. Support is added to the RenderText class to retreive the word displayed at a given point along with its styling information. A class DecoratedText is introduced to encapsulate the styling information for some given text. Note, this CL only adds support for dictionary popups to Views::Textfields. Doc- https://docs.google.com/a/google.com/document/d/17QYrMpIXpk-FpGanjhJ-Yx33njNQ.... BUG=640502 ==========
Description was changed from ========== This CL adds supports to MacViews for Force touch and dictionary popups. This is achieved by implement quickLookWithEvent: on the BridgedContentView. Methods are added to views::View to retreive the underlying RenderText instance. Support is added to the RenderText class to retreive the word displayed at a given point along with its styling information. A class DecoratedText is introduced to encapsulate the styling information for some given text. Note, this CL only adds support for dictionary popups to Views::Textfields. Doc- https://docs.google.com/a/google.com/document/d/17QYrMpIXpk-FpGanjhJ-Yx33njNQ.... BUG=640502 ========== to ========== This CL adds supports to MacViews for Force touch and dictionary popups. This is achieved by implement quickLookWithEvent: on the BridgedContentView. Methods are added to views::View to retreive the underlying RenderText instance. Support is added to the RenderText class to retreive the word displayed at a given point along with its styling information. A DecoratedText class is introduced to encapsulate the styling information for some given text. Note, this CL only adds support for dictionary popups to Views::Textfields. Doc- https://docs.google.com/a/google.com/document/d/17QYrMpIXpk-FpGanjhJ-Yx33njNQ.... BUG=640502 ==========
Description was changed from ========== This CL adds supports to MacViews for Force touch and dictionary popups. This is achieved by implement quickLookWithEvent: on the BridgedContentView. Methods are added to views::View to retreive the underlying RenderText instance. Support is added to the RenderText class to retreive the word displayed at a given point along with its styling information. A DecoratedText class is introduced to encapsulate the styling information for some given text. Note, this CL only adds support for dictionary popups to Views::Textfields. Doc- https://docs.google.com/a/google.com/document/d/17QYrMpIXpk-FpGanjhJ-Yx33njNQ.... BUG=640502 ========== to ========== This CL adds supports to MacViews for Force touch and dictionary popups. This is achieved by implement quickLookWithEvent: on the BridgedContentView. Methods are added to views::View to retreive the underlying RenderText instance. Support is added to the RenderText class to retreive the word displayed at a given point along with its styling information. A DecoratedText class is introduced to encapsulate the styling information for some given text. Note, this CL only adds support for dictionary popups to Views::Textfields. Doc- https://docs.google.com/a/google.com/document/d/17QYrMpIXpk-FpGanjhJ-Yx33njNQ.... BUG=640502 TEST= Enable chrome://flags/#mac-views-native-dialogs. Open Bookmark Bubble. Enter some text in the name textfield. Hover over the text and press Ctrl+Command+D. Verify that a dictionary popup appears showing the definition of the word, the mouse was hovering on. ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2348143003/diff/360001/ui/views/view.h File ui/views/view.h (right): https://codereview.chromium.org/2348143003/diff/360001/ui/views/view.h#newcod... ui/views/view.h:579: virtual const gfx::RenderText* GetRenderText() const; Can you outline why this needs to be exposed on View? I would prefer consumers that need to know the type of View they have and operate on it.
PTAL sky@. https://codereview.chromium.org/2348143003/diff/360001/ui/views/view.h File ui/views/view.h (right): https://codereview.chromium.org/2348143003/diff/360001/ui/views/view.h#newcod... ui/views/view.h:579: virtual const gfx::RenderText* GetRenderText() const; On 2016/09/26 16:20:18, sky wrote: > Can you outline why this needs to be exposed on View? I would prefer consumers > that need to know the type of View they have and operate on it. So the problem we want to solve is given a views::View, get the word being displayed at some given point. This is only needed for Textfields and Labels currently. Possible approaches- 1) Expose GetRenderText on View. 2) Expose GetWordAtPoint on View. (Subsequently we might also need something like GetWordForRange). 3) Expose GetTextInputClient on View and add GetWordAtPoint to the TextInputClient interface. (But Labels don't implement the TextInputClient interface). 4) Use RTTI at the call site (BridgedContentView) with the help of View::GetClassName() and static_cast. I have also outlined these options on the linked doc - https://docs.google.com/document/d/17QYrMpIXpk-FpGanjhJ-Yx33njNQy7B5oL_bxb7-v...
None of these options are that satisfying. I favor 4 as it's what we've done in similar situations. -Scott On Mon, Sep 26, 2016 at 6:09 PM, <karandeepb@chromium.org> wrote: > PTAL sky@. > > > https://codereview.chromium.org/2348143003/diff/360001/ui/views/view.h > File ui/views/view.h (right): > > https://codereview.chromium.org/2348143003/diff/360001/ui/views/view.h#newcod... > ui/views/view.h:579: virtual const gfx::RenderText* GetRenderText() > const; > On 2016/09/26 16:20:18, sky wrote: >> Can you outline why this needs to be exposed on View? I would prefer > consumers >> that need to know the type of View they have and operate on it. > > So the problem we want to solve is given a views::View, get the word > being displayed at some given point. This is only needed for Textfields > and Labels currently. Possible approaches- > 1) Expose GetRenderText on View. > 2) Expose GetWordAtPoint on View. (Subsequently we might also need > something like GetWordForRange). > 3) Expose GetTextInputClient on View and add GetWordAtPoint to the > TextInputClient interface. (But Labels don't implement the > TextInputClient interface). > 4) Use RTTI at the call site (BridgedContentView) with the help of > View::GetClassName() and static_cast. > > I have also outlined these options on the linked doc - > https://docs.google.com/document/d/17QYrMpIXpk-FpGanjhJ-Yx33njNQy7B5oL_bxb7-v... > > https://codereview.chromium.org/2348143003/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/09/27 13:15:34, sky wrote: > None of these options are that satisfying. I favor 4 as it's what > we've done in similar situations. > > -Scott > > On Mon, Sep 26, 2016 at 6:09 PM, <mailto:karandeepb@chromium.org> wrote: > > PTAL sky@. > > > > > > https://codereview.chromium.org/2348143003/diff/360001/ui/views/view.h > > File ui/views/view.h (right): > > > > > https://codereview.chromium.org/2348143003/diff/360001/ui/views/view.h#newcod... > > ui/views/view.h:579: virtual const gfx::RenderText* GetRenderText() > > const; > > On 2016/09/26 16:20:18, sky wrote: > >> Can you outline why this needs to be exposed on View? I would prefer > > consumers > >> that need to know the type of View they have and operate on it. > > > > So the problem we want to solve is given a views::View, get the word > > being displayed at some given point. This is only needed for Textfields > > and Labels currently. Possible approaches- > > 1) Expose GetRenderText on View. > > 2) Expose GetWordAtPoint on View. (Subsequently we might also need > > something like GetWordForRange). > > 3) Expose GetTextInputClient on View and add GetWordAtPoint to the > > TextInputClient interface. (But Labels don't implement the > > TextInputClient interface). > > 4) Use RTTI at the call site (BridgedContentView) with the help of > > View::GetClassName() and static_cast. > > > > I have also outlined these options on the linked doc - > > > https://docs.google.com/document/d/17QYrMpIXpk-FpGanjhJ-Yx33njNQy7B5oL_bxb7-v... > > > > https://codereview.chromium.org/2348143003/ > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. > Somehow I didn't get a notification for your reply. I think using RTTI has its own set of problems. If we use RTTI, separate handling will be needed for subclasses of Textfield/Labels at the call site. Also, for example the call site in this case is the BridgedContentView, which resides in the //ui/views:views target. So it won't be possible to refer to the class name of a view like the OmniboxViewViews (which resides in //chrome/browser/ui:ui). Doesn't GetRenderText seem generic enough to be added to the View class?
The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run
Patchset #10 (id:400001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #11 (id:440001) has been deleted
Ping sky@ regarding c#62. Also PTAL msw@. I have now added some tests as well. I think the only outstanding issue is whether we want to expose RenderText from View or use RTTI at call sites. The rest of the patch should be ready for review. https://codereview.chromium.org/2348143003/diff/380001/ui/gfx/render_text_har... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/2348143003/diff/380001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:852: Font font(family_name.c_str(), run->font_size); Any reason why run->font wasn't used here, instead of creating the font from family name.
On 2016/09/29 04:35:01, karandeepb wrote: > On 2016/09/27 13:15:34, sky wrote: > > None of these options are that satisfying. I favor 4 as it's what > > we've done in similar situations. > > > > -Scott > > > > On Mon, Sep 26, 2016 at 6:09 PM, <mailto:karandeepb@chromium.org> wrote: > > > PTAL sky@. > > > > > > > > > https://codereview.chromium.org/2348143003/diff/360001/ui/views/view.h > > > File ui/views/view.h (right): > > > > > > > > > https://codereview.chromium.org/2348143003/diff/360001/ui/views/view.h#newcod... > > > ui/views/view.h:579: virtual const gfx::RenderText* GetRenderText() > > > const; > > > On 2016/09/26 16:20:18, sky wrote: > > >> Can you outline why this needs to be exposed on View? I would prefer > > > consumers > > >> that need to know the type of View they have and operate on it. > > > > > > So the problem we want to solve is given a views::View, get the word > > > being displayed at some given point. This is only needed for Textfields > > > and Labels currently. Possible approaches- > > > 1) Expose GetRenderText on View. > > > 2) Expose GetWordAtPoint on View. (Subsequently we might also need > > > something like GetWordForRange). > > > 3) Expose GetTextInputClient on View and add GetWordAtPoint to the > > > TextInputClient interface. (But Labels don't implement the > > > TextInputClient interface). > > > 4) Use RTTI at the call site (BridgedContentView) with the help of > > > View::GetClassName() and static_cast. > > > > > > I have also outlined these options on the linked doc - > > > > > > https://docs.google.com/document/d/17QYrMpIXpk-FpGanjhJ-Yx33njNQy7B5oL_bxb7-v... > > > > > > https://codereview.chromium.org/2348143003/ > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > Somehow I didn't get a notification for your reply. > > I think using RTTI has its own set of problems. If we use RTTI, separate > handling will be needed for subclasses of Textfield/Labels at the call site. > Also, for example the call site in this case is the BridgedContentView, which > resides in the //ui/views:views target. So it won't be possible to refer to the > class name of a view like the OmniboxViewViews (which resides in > //chrome/browser/ui:ui). > > Doesn't GetRenderText seem generic enough to be added to the View class? GetRenderText is specific to only some subclasses, and it's questionable whether subclasses would want to expose it at all. I still tend to favor 4, but I'm ok with a new interface that contains the query functions you need and a getter on view. RenderText is too generic.
https://codereview.chromium.org/2348143003/diff/500001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/2348143003/diff/500001/ui/gfx/render_text.cc#... ui/gfx/render_text.cc:1050: if (range.IsValid() && range.GetMin() < text().length()) I see that you simply moved this code, but maybe it should check that range.GetMax() <= text().length()? Otherwise, can you check that substr() does the right thing if the length provided exceeds the length of the string? https://codereview.chromium.org/2348143003/diff/500001/ui/gfx/render_text.cc#... ui/gfx/render_text.cc:1051: return text().substr(range.GetMin(), range.length()); This may split multi-char glyphs, yielding an invalid string, is that okay? Should we dcheck against that? https://codereview.chromium.org/2348143003/diff/500001/ui/gfx/render_text.cc#... ui/gfx/render_text.cc:1642: if (obscured()) nit: return |length| if |length == 0| https://codereview.chromium.org/2348143003/diff/500001/ui/gfx/render_text.cc#... ui/gfx/render_text.cc:1653: for (int i = std::min<int>(index, (int)length - 1); i >= 0; i--) nit: static_cast<int>(length) if necessary https://codereview.chromium.org/2348143003/diff/500001/ui/gfx/render_text.cc#... ui/gfx/render_text.cc:1666: DCHECK(range.GetMax() <= length); optional nit: DCHECK_LE https://codereview.chromium.org/2348143003/diff/500001/ui/gfx/render_text.cc#... ui/gfx/render_text.cc:1680: for (; range_min != 0; --range_min) { nit: remove curlies here (or add them for the corresponding range_max loop) https://codereview.chromium.org/2348143003/diff/500001/ui/gfx/render_text_har... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/2348143003/diff/500001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:1612: Font font_with_style = run.font.Derive(0, style, run.weight); optional nit: inline below in DecoratedText::RangedAttribute ctor arg https://codereview.chromium.org/2348143003/diff/500001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:1615: DecoratedText::RangedAttribute attribute( producing a range for every run might not give the most efficient result; runs are split on more than just style changes (ie. block codes for symbols, etc. see FindRunBreakingCharacter), so you might wind up with 2+ DecoratedText::RangedAttribute instances when one would suffice. Is that okay? I would suggest building these structs from the break lists instead (via StyleIterator), but that doesn't support strikes and diagonal strikes at the moment. https://codereview.chromium.org/2348143003/diff/500001/ui/gfx/render_text_mac.mm File ui/gfx/render_text_mac.mm (right): https://codereview.chromium.org/2348143003/diff/500001/ui/gfx/render_text_mac... ui/gfx/render_text_mac.mm:459: decorated_text->attributes.clear(); q: is this necessary? https://codereview.chromium.org/2348143003/diff/500001/ui/gfx/render_text_uni... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/2348143003/diff/500001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:218: // Creates a RangedAttribute instance for a given |index|, |weight| and nit: 'for a single character range at the given |index|' https://codereview.chromium.org/2348143003/diff/500001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:233: DCHECK(font_spans.end() != iter); nit: DCHECK_NE https://codereview.chromium.org/2348143003/diff/500001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:259: auto find_attribute_func = [i](const DecoratedText::RangedAttribute& attr) { Can you just loop over the expected.attributes and compare them to the actual.attributes by the corresponding index number and compare their ranges, instead of looping over the text indices and frequently comparing the same ranged attributes multiple times? https://codereview.chromium.org/2348143003/diff/500001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:273: EXPECT_EQ(expected_attr->font.GetFontName(), Can we just compare the Font objects themselves? https://codereview.chromium.org/2348143003/diff/500001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:3902: for (size_t i = 0; i < render_text->text().length(); i++) { nit: add a test that checks the behavior with points outside the text bounds. https://codereview.chromium.org/2348143003/diff/500001/ui/views/controls/text... File ui/views/controls/textfield/textfield_model.h (right): https://codereview.chromium.org/2348143003/diff/500001/ui/views/controls/text... ui/views/controls/textfield/textfield_model.h:209: return render_text_->GetTextFromRange(range); nit: I think this still belongs in the .cc file
The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #17 (id:580001) has been deleted
The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL sky@, msw@. Have created a new interface LookupWordClient with a getter on the View class. https://codereview.chromium.org/2348143003/diff/500001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/2348143003/diff/500001/ui/gfx/render_text.cc#... ui/gfx/render_text.cc:1050: if (range.IsValid() && range.GetMin() < text().length()) On 2016/10/04 01:52:37, msw wrote: > I see that you simply moved this code, but maybe it should check that > range.GetMax() <= text().length()? Otherwise, can you check that substr() does > the right thing if the length provided exceeds the length of the string? Checked that substr does the right thing (return the remainder of the string from range.GetMin()) when length provided exceeds the remaining length of the string. https://codereview.chromium.org/2348143003/diff/500001/ui/gfx/render_text.cc#... ui/gfx/render_text.cc:1051: return text().substr(range.GetMin(), range.length()); On 2016/10/04 01:52:37, msw wrote: > This may split multi-char glyphs, yielding an invalid string, is that okay? > Should we dcheck against that? Currently, Textfield::GetTextFromRange does not check for this. Also, the TextInputClient interface does not say anything about this case. I guess the options are- 1) If the range is invalid, extend the range so that it corresponds to valid cursor indices. But then we should also return the actual range used. 2) Make RenderText::GetTextFromRange return a bool and return false if the range does not correspond to valid cursor indices. 3) Have the clients deal with it. But For 1 and 2, the clients which call into Textfield::GetTextFromRange will also need to be audited, since there is behavior change involved. Hence I think for this patch at least, we should go with 3 currently. https://codereview.chromium.org/2348143003/diff/500001/ui/gfx/render_text.cc#... ui/gfx/render_text.cc:1642: if (obscured()) On 2016/10/04 01:52:37, msw wrote: > nit: return |length| if |length == 0| Done. https://codereview.chromium.org/2348143003/diff/500001/ui/gfx/render_text.cc#... ui/gfx/render_text.cc:1653: for (int i = std::min<int>(index, (int)length - 1); i >= 0; i--) On 2016/10/04 01:52:37, msw wrote: > nit: static_cast<int>(length) if necessary Removed the cast, which is not necessary now since length != 0. https://codereview.chromium.org/2348143003/diff/500001/ui/gfx/render_text.cc#... ui/gfx/render_text.cc:1666: DCHECK(range.GetMax() <= length); On 2016/10/04 01:52:37, msw wrote: > optional nit: DCHECK_LE Done. https://codereview.chromium.org/2348143003/diff/500001/ui/gfx/render_text.cc#... ui/gfx/render_text.cc:1680: for (; range_min != 0; --range_min) { On 2016/10/04 01:52:37, msw wrote: > nit: remove curlies here (or add them for the corresponding range_max loop) Done. https://codereview.chromium.org/2348143003/diff/500001/ui/gfx/render_text_har... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/2348143003/diff/500001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:1612: Font font_with_style = run.font.Derive(0, style, run.weight); On 2016/10/04 01:52:37, msw wrote: > optional nit: inline below in DecoratedText::RangedAttribute ctor arg Done. https://codereview.chromium.org/2348143003/diff/500001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:1615: DecoratedText::RangedAttribute attribute( On 2016/10/04 01:52:37, msw wrote: > producing a range for every run might not give the most efficient result; runs > are split on more than just style changes (ie. block codes for symbols, etc. see > FindRunBreakingCharacter), so you might wind up with 2+ > DecoratedText::RangedAttribute instances when one would suffice. Is that okay? I > would suggest building these structs from the break lists instead (via > StyleIterator), but that doesn't support strikes and diagonal strikes at the > moment. It seems StyleIterator does support strikes and diagonal strikes. However, I also need the font as part of the RangedAttribute and I think we can retrieve the font only from the text runs. https://codereview.chromium.org/2348143003/diff/500001/ui/gfx/render_text_mac.mm File ui/gfx/render_text_mac.mm (right): https://codereview.chromium.org/2348143003/diff/500001/ui/gfx/render_text_mac... ui/gfx/render_text_mac.mm:459: decorated_text->attributes.clear(); On 2016/10/04 01:52:37, msw wrote: > q: is this necessary? Not really, removed. https://codereview.chromium.org/2348143003/diff/500001/ui/gfx/render_text_uni... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/2348143003/diff/500001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:218: // Creates a RangedAttribute instance for a given |index|, |weight| and On 2016/10/04 01:52:37, msw wrote: > nit: 'for a single character range at the given |index|' Done. https://codereview.chromium.org/2348143003/diff/500001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:233: DCHECK(font_spans.end() != iter); On 2016/10/04 01:52:37, msw wrote: > nit: DCHECK_NE That results in a compile error since the arguments don't have operator << defined. https://codereview.chromium.org/2348143003/diff/500001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:259: auto find_attribute_func = [i](const DecoratedText::RangedAttribute& attr) { On 2016/10/04 01:52:37, msw wrote: > Can you just loop over the expected.attributes and compare them to the > actual.attributes by the corresponding index number and compare their ranges, > instead of looping over the text indices and frequently comparing the same > ranged attributes multiple times? The partition into ranges for the actual DecoratedText will depend on the text runs. Hence the corresponding RangedAttributes can't be compared. Have added a comment explaining this. https://codereview.chromium.org/2348143003/diff/500001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:273: EXPECT_EQ(expected_attr->font.GetFontName(), On 2016/10/04 01:52:37, msw wrote: > Can we just compare the Font objects themselves? Both Font and the underlying PlatformFont don't overload the == operator. Ideally we should have been able to compare the underlying PlatformFont pointers, but these are different in |expected_attr| and |actual_attr| due to the intermediate use of DeriveFont. https://codereview.chromium.org/2348143003/diff/500001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:3902: for (size_t i = 0; i < render_text->text().length(); i++) { On 2016/10/04 01:52:37, msw wrote: > nit: add a test that checks the behavior with points outside the text bounds. Done. https://codereview.chromium.org/2348143003/diff/500001/ui/views/controls/text... File ui/views/controls/textfield/textfield_model.h (right): https://codereview.chromium.org/2348143003/diff/500001/ui/views/controls/text... ui/views/controls/textfield/textfield_model.h:209: return render_text_->GetTextFromRange(range); On 2016/10/04 01:52:37, msw wrote: > nit: I think this still belongs in the .cc file Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2348143003/diff/600001/ui/views/lookup_word_c... File ui/views/lookup_word_client.h (right): https://codereview.chromium.org/2348143003/diff/600001/ui/views/lookup_word_c... ui/views/lookup_word_client.h:18: class VIEWS_EXPORT LookupWordClient { WordLookupClient?
Please update the CL description (no more View::GetRenderText); otherwise lgtm with nits. https://codereview.chromium.org/2348143003/diff/500001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/2348143003/diff/500001/ui/gfx/render_text.cc#... ui/gfx/render_text.cc:1050: if (range.IsValid() && range.GetMin() < text().length()) On 2016/10/04 12:04:24, karandeepb wrote: > On 2016/10/04 01:52:37, msw wrote: > > I see that you simply moved this code, but maybe it should check that > > range.GetMax() <= text().length()? Otherwise, can you check that substr() does > > the right thing if the length provided exceeds the length of the string? > > Checked that substr does the right thing (return the remainder of the string > from range.GetMin()) when length provided exceeds the remaining length of the > string. Acknowledged; thanks. https://codereview.chromium.org/2348143003/diff/500001/ui/gfx/render_text.cc#... ui/gfx/render_text.cc:1051: return text().substr(range.GetMin(), range.length()); On 2016/10/04 12:04:24, karandeepb wrote: > On 2016/10/04 01:52:37, msw wrote: > > This may split multi-char glyphs, yielding an invalid string, is that okay? > > Should we dcheck against that? > > Currently, Textfield::GetTextFromRange does not check for this. Also, the > TextInputClient interface does not say anything about this case. I guess the > options are- > 1) If the range is invalid, extend the range so that it corresponds to valid > cursor indices. But then we should also return the actual range used. > 2) Make RenderText::GetTextFromRange return a bool and return false if the range > does not correspond to valid cursor indices. > 3) Have the clients deal with it. > > But For 1 and 2, the clients which call into Textfield::GetTextFromRange will > also need to be audited, since there is behavior change involved. Hence I think > for this patch at least, we should go with 3 currently. Acknowledged. https://codereview.chromium.org/2348143003/diff/500001/ui/gfx/render_text_har... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/2348143003/diff/500001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:1615: DecoratedText::RangedAttribute attribute( On 2016/10/04 12:04:24, karandeepb wrote: > On 2016/10/04 01:52:37, msw wrote: > > producing a range for every run might not give the most efficient result; runs > > are split on more than just style changes (ie. block codes for symbols, etc. > see > > FindRunBreakingCharacter), so you might wind up with 2+ > > DecoratedText::RangedAttribute instances when one would suffice. Is that okay? > I > > would suggest building these structs from the break lists instead (via > > StyleIterator), but that doesn't support strikes and diagonal strikes at the > > moment. > > It seems StyleIterator does support strikes and diagonal strikes. However, I > also need the font as part of the RangedAttribute and I think we can retrieve > the font only from the text runs. You're right, it does handle strikes as styles, but it doesn't handle Font. https://codereview.chromium.org/2348143003/diff/500001/ui/gfx/render_text_uni... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/2348143003/diff/500001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:259: auto find_attribute_func = [i](const DecoratedText::RangedAttribute& attr) { On 2016/10/04 12:04:25, karandeepb wrote: > On 2016/10/04 01:52:37, msw wrote: > > Can you just loop over the expected.attributes and compare them to the > > actual.attributes by the corresponding index number and compare their ranges, > > instead of looping over the text indices and frequently comparing the same > > ranged attributes multiple times? > > The partition into ranges for the actual DecoratedText will depend on the text > runs. Hence the corresponding RangedAttributes can't be compared. Have added a > comment explaining this. Acknowledged. https://codereview.chromium.org/2348143003/diff/500001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:273: EXPECT_EQ(expected_attr->font.GetFontName(), On 2016/10/04 12:04:25, karandeepb wrote: > On 2016/10/04 01:52:37, msw wrote: > > Can we just compare the Font objects themselves? > > Both Font and the underlying PlatformFont don't overload the == operator. > Ideally we should have been able to compare the underlying PlatformFont > pointers, but these are different in |expected_attr| and |actual_attr| due to > the intermediate use of DeriveFont. Acknowledged. https://codereview.chromium.org/2348143003/diff/600001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/2348143003/diff/600001/ui/gfx/render_text.cc#... ui/gfx/render_text.cc:1034: !GetDecoratedTextForRange(word_range, decorated_word)) nit: curlies https://codereview.chromium.org/2348143003/diff/600001/ui/views/lookup_word_c... File ui/views/lookup_word_client.h (right): https://codereview.chromium.org/2348143003/diff/600001/ui/views/lookup_word_c... ui/views/lookup_word_client.h:20: // Retreives the word displayed at the given |point| along with its styling nit: Retrieves https://codereview.chromium.org/2348143003/diff/600001/ui/views/lookup_word_c... ui/views/lookup_word_client.h:23: // correspond to the left baseline point of the leftmost glyph of the |word| nit: s/the left baseline/the baseline/
Description was changed from ========== This CL adds supports to MacViews for Force touch and dictionary popups. This is achieved by implement quickLookWithEvent: on the BridgedContentView. Methods are added to views::View to retreive the underlying RenderText instance. Support is added to the RenderText class to retreive the word displayed at a given point along with its styling information. A DecoratedText class is introduced to encapsulate the styling information for some given text. Note, this CL only adds support for dictionary popups to Views::Textfields. Doc- https://docs.google.com/a/google.com/document/d/17QYrMpIXpk-FpGanjhJ-Yx33njNQ.... BUG=640502 TEST= Enable chrome://flags/#mac-views-native-dialogs. Open Bookmark Bubble. Enter some text in the name textfield. Hover over the text and press Ctrl+Command+D. Verify that a dictionary popup appears showing the definition of the word, the mouse was hovering on. ========== to ========== MacViews: Implement Force Touch/Mac dictionary lookups for Textfields. This CL adds support to MacViews for Force touch and dictionary lookups. This is achieved by implementing quickLookWithEvent: on the BridgedContentView. A new interface called WordLookupClient is added which is to be implemented by views which support word lookups. Currently only views::Textfield implements this interface. A virtual method GetWordLookupClient is added to views::View to retrieve the WordLookupClient associated with a given View. Support is added to the RenderText class to retreive the word displayed at a given point along with its styling information. A DecoratedText class is also introduced to encapsulate the styling information for some given text. Note, this CL only adds support for dictionary lookups to Views::Textfields. Doc- https://docs.google.com/a/google.com/document/d/17QYrMpIXpk-FpGanjhJ-Yx33njNQ.... BUG=640502 TEST= Enable chrome://flags/#mac-views-native-dialogs. Open Bookmark Bubble. Enter some text in the name textfield. Hover over the text and press Ctrl+Command+D. Verify that a dictionary popup appears showing the definition of the word, the mouse was hovering on. ==========
Description was changed from ========== MacViews: Implement Force Touch/Mac dictionary lookups for Textfields. This CL adds support to MacViews for Force touch and dictionary lookups. This is achieved by implementing quickLookWithEvent: on the BridgedContentView. A new interface called WordLookupClient is added which is to be implemented by views which support word lookups. Currently only views::Textfield implements this interface. A virtual method GetWordLookupClient is added to views::View to retrieve the WordLookupClient associated with a given View. Support is added to the RenderText class to retreive the word displayed at a given point along with its styling information. A DecoratedText class is also introduced to encapsulate the styling information for some given text. Note, this CL only adds support for dictionary lookups to Views::Textfields. Doc- https://docs.google.com/a/google.com/document/d/17QYrMpIXpk-FpGanjhJ-Yx33njNQ.... BUG=640502 TEST= Enable chrome://flags/#mac-views-native-dialogs. Open Bookmark Bubble. Enter some text in the name textfield. Hover over the text and press Ctrl+Command+D. Verify that a dictionary popup appears showing the definition of the word, the mouse was hovering on. ========== to ========== MacViews: Implement Force Touch/Mac dictionary lookup for Textfields. This CL adds support to MacViews for Force touch and dictionary lookups. This is achieved by implementing quickLookWithEvent: on the BridgedContentView. A new interface called WordLookupClient is added which is to be implemented by views which support word lookups. Currently only views::Textfield implements this interface. A virtual method GetWordLookupClient is added to views::View to retrieve the WordLookupClient associated with a given View. Support is added to the RenderText class to retreive the word displayed at a given point along with its styling information. A DecoratedText class is also introduced to encapsulate the styling information for some given text. Note, this CL only adds support for dictionary lookups to Views::Textfields. Doc- https://docs.google.com/a/google.com/document/d/17QYrMpIXpk-FpGanjhJ-Yx33njNQ.... BUG=640502 TEST= Enable chrome://flags/#mac-views-native-dialogs. Open Bookmark Bubble. Enter some text in the name textfield. Hover over the text and press Ctrl+Command+D. Verify that a dictionary popup appears showing the definition of the word, the mouse was hovering on. ==========
Patchset #18 (id:620001) has been deleted
The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for the review msw@. Have also changed GetDecoratedTextForRange to call EnsureLayout first. https://codereview.chromium.org/2348143003/diff/600001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/2348143003/diff/600001/ui/gfx/render_text.cc#... ui/gfx/render_text.cc:1034: !GetDecoratedTextForRange(word_range, decorated_word)) On 2016/10/05 00:24:49, msw wrote: > nit: curlies Done. https://codereview.chromium.org/2348143003/diff/600001/ui/views/lookup_word_c... File ui/views/lookup_word_client.h (right): https://codereview.chromium.org/2348143003/diff/600001/ui/views/lookup_word_c... ui/views/lookup_word_client.h:20: // Retreives the word displayed at the given |point| along with its styling On 2016/10/05 00:24:49, msw wrote: > nit: Retrieves Done. https://codereview.chromium.org/2348143003/diff/600001/ui/views/lookup_word_c... ui/views/lookup_word_client.h:23: // correspond to the left baseline point of the leftmost glyph of the |word| On 2016/10/05 00:24:49, msw wrote: > nit: s/the left baseline/the baseline/ Done.
PTAL sky@ and Trent. https://codereview.chromium.org/2348143003/diff/600001/ui/views/lookup_word_c... File ui/views/lookup_word_client.h (right): https://codereview.chromium.org/2348143003/diff/600001/ui/views/lookup_word_c... ui/views/lookup_word_client.h:18: class VIEWS_EXPORT LookupWordClient { On 2016/10/04 17:26:47, sky wrote: > WordLookupClient? Done.
lgtm, just a few nits https://codereview.chromium.org/2348143003/diff/660001/ui/gfx/render_text_har... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/2348143003/diff/660001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:1617: gfx::Range(intersection.start() - range.GetMin(), nit: gfx:: not required https://codereview.chromium.org/2348143003/diff/660001/ui/gfx/render_text_uni... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/2348143003/diff/660001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:244: DecoratedText::RangedAttribute attributes(gfx::Range(index, index + 1), nit: no gfx::, more below (gfx::Rect, gfx::Range, gfx::Point) https://codereview.chromium.org/2348143003/diff/660001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:3912: VerifyDecoratedWordsAreEqual(expected_word_1, decorated_word); nit: wrapping calls to VerifyDecoratedWordsAreEqual in EXPECT_NO_FATAL_FAILURE will allow a line number to be emitted here in case of any failure (otherwise, it's a little more effort to differentiate this call from the one below) https://codereview.chromium.org/2348143003/diff/660001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:4020: TEST_P(RenderTextHarfBuzzTest, GetDecoratedWordAtPoint_Return) { can this be a RenderTextTest already, since it doesn't rely on the information in the harfbuzz runs? https://codereview.chromium.org/2348143003/diff/660001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2348143003/diff/660001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:219: [[str mutableString] setString:base::SysUTF16ToNSString(decorated_text.text)]; I haven't seen this approach in Chrome before - I think it would be nicer to just initialise with the string. I.e., [[NSMutableAttributedString alloc] initWithString:base::SysUTF16ToNSString(decorated_text.text)] https://codereview.chromium.org/2348143003/diff/660001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:236: @(NSUnderlineStyleSingle | NSUnderlinePatternSolid); How about, at the top of the method, NSValue* const line_style = @(NSUnderlineStyleSingle | NSUnderlinePatternSolid); https://codereview.chromium.org/2348143003/diff/660001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:793: views::WordLookupClient* word_lookup_client = target->GetWordLookupClient(); word_lookup_client -> wordLookupClient https://codereview.chromium.org/2348143003/diff/660001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:799: gfx::DecoratedText decorated_word; decorated_word -> decoratedWord https://codereview.chromium.org/2348143003/diff/660001/ui/views/word_lookup_c... File ui/views/word_lookup_client.h (right): https://codereview.chromium.org/2348143003/diff/660001/ui/views/word_lookup_c... ui/views/word_lookup_client.h:13: } // namespace gfx nit: comments aren't usually put at the end of namespaces filled just with forward declares https://codereview.chromium.org/2348143003/diff/660001/ui/views/word_lookup_c... ui/views/word_lookup_client.h:30: ~WordLookupClient() {} declare virtual? (or can it be omitted whilst there are no members?)
LGTM https://codereview.chromium.org/2348143003/diff/660001/ui/views/word_lookup_c... File ui/views/word_lookup_client.h (right): https://codereview.chromium.org/2348143003/diff/660001/ui/views/word_lookup_c... ui/views/word_lookup_client.h:30: ~WordLookupClient() {} On 2016/10/05 06:20:55, tapted wrote: > declare virtual? (or can it be omitted whilst there are no members?) Generally we declare the destructor in pure virtual interfaces, and you are right it should be virtual.
Patchset #20 (id:680001) has been deleted
https://codereview.chromium.org/2348143003/diff/660001/ui/gfx/render_text_har... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/2348143003/diff/660001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:1617: gfx::Range(intersection.start() - range.GetMin(), On 2016/10/05 06:20:54, tapted wrote: > nit: gfx:: not required Done. https://codereview.chromium.org/2348143003/diff/660001/ui/gfx/render_text_uni... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/2348143003/diff/660001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:244: DecoratedText::RangedAttribute attributes(gfx::Range(index, index + 1), On 2016/10/05 06:20:54, tapted wrote: > nit: no gfx::, more below (gfx::Rect, gfx::Range, gfx::Point) Done. https://codereview.chromium.org/2348143003/diff/660001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:3912: VerifyDecoratedWordsAreEqual(expected_word_1, decorated_word); On 2016/10/05 06:20:54, tapted wrote: > nit: wrapping calls to VerifyDecoratedWordsAreEqual in EXPECT_NO_FATAL_FAILURE > will allow a line number to be emitted here in case of any failure (otherwise, > it's a little more effort to differentiate this call from the one below) But that'll only work for a fatal failure right? Have wrapped these with a SCOPED_TRACE. https://codereview.chromium.org/2348143003/diff/660001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:4020: TEST_P(RenderTextHarfBuzzTest, GetDecoratedWordAtPoint_Return) { On 2016/10/05 06:20:54, tapted wrote: > can this be a RenderTextTest already, since it doesn't rely on the information > in the harfbuzz runs? For an RTM instance, GetDecoratedWordAtPoint will always return false, since the word_bounds will be empty and hence we'll never reach RTM::GetDecoratedTextForRange. https://codereview.chromium.org/2348143003/diff/660001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2348143003/diff/660001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:219: [[str mutableString] setString:base::SysUTF16ToNSString(decorated_text.text)]; On 2016/10/05 06:20:54, tapted wrote: > I haven't seen this approach in Chrome before - I think it would be nicer to > just initialise with the string. I.e., > > [[NSMutableAttributedString alloc] > initWithString:base::SysUTF16ToNSString(decorated_text.text)] Done. https://codereview.chromium.org/2348143003/diff/660001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:236: @(NSUnderlineStyleSingle | NSUnderlinePatternSolid); On 2016/10/05 06:20:55, tapted wrote: > How about, at the top of the method, > > NSValue* const line_style = > @(NSUnderlineStyleSingle | NSUnderlinePatternSolid); Done. https://codereview.chromium.org/2348143003/diff/660001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:793: views::WordLookupClient* word_lookup_client = target->GetWordLookupClient(); On 2016/10/05 06:20:54, tapted wrote: > word_lookup_client -> wordLookupClient Done. https://codereview.chromium.org/2348143003/diff/660001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:799: gfx::DecoratedText decorated_word; On 2016/10/05 06:20:55, tapted wrote: > decorated_word -> decoratedWord Done. https://codereview.chromium.org/2348143003/diff/660001/ui/views/word_lookup_c... File ui/views/word_lookup_client.h (right): https://codereview.chromium.org/2348143003/diff/660001/ui/views/word_lookup_c... ui/views/word_lookup_client.h:13: } // namespace gfx On 2016/10/05 06:20:55, tapted wrote: > nit: comments aren't usually put at the end of namespaces filled just with > forward declares Done. https://codereview.chromium.org/2348143003/diff/660001/ui/views/word_lookup_c... ui/views/word_lookup_client.h:30: ~WordLookupClient() {} On 2016/10/05 16:15:36, sky wrote: > On 2016/10/05 06:20:55, tapted wrote: > > declare virtual? (or can it be omitted whilst there are no members?) > > Generally we declare the destructor in pure virtual interfaces, and you are > right it should be virtual. Done. https://codereview.chromium.org/2348143003/diff/660001/ui/views/word_lookup_c... ui/views/word_lookup_client.h:30: ~WordLookupClient() {} On 2016/10/05 06:20:55, tapted wrote: > declare virtual? (or can it be omitted whilst there are no members?) Declared virtual. I think if we omit the destructor, it would be public and non-virtual. Also, just for my own understanding, is declaring it virtual wrong, if I don't expect the instance to be deleted through a base class pointer and if there is no "delete this" in the base class implementation.
still lgtm https://codereview.chromium.org/2348143003/diff/660001/ui/gfx/render_text_uni... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/2348143003/diff/660001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:3912: VerifyDecoratedWordsAreEqual(expected_word_1, decorated_word); On 2016/10/06 01:07:05, karandeepb wrote: > On 2016/10/05 06:20:54, tapted wrote: > > nit: wrapping calls to VerifyDecoratedWordsAreEqual in EXPECT_NO_FATAL_FAILURE > > will allow a line number to be emitted here in case of any failure (otherwise, > > it's a little more effort to differentiate this call from the one below) > > But that'll only work for a fatal failure right? Have wrapped these with a > SCOPED_TRACE. Acknowledged. Yeah - it does look like gtest is missing a nice thing to scope a failed EXCEPT :/ https://codereview.chromium.org/2348143003/diff/660001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:4020: TEST_P(RenderTextHarfBuzzTest, GetDecoratedWordAtPoint_Return) { On 2016/10/06 01:07:05, karandeepb wrote: > On 2016/10/05 06:20:54, tapted wrote: > > can this be a RenderTextTest already, since it doesn't rely on the information > > in the harfbuzz runs? > > For an RTM instance, GetDecoratedWordAtPoint will always return false, since the > word_bounds will be empty and hence we'll never reach > RTM::GetDecoratedTextForRange. Acknowledged. https://codereview.chromium.org/2348143003/diff/660001/ui/views/word_lookup_c... File ui/views/word_lookup_client.h (right): https://codereview.chromium.org/2348143003/diff/660001/ui/views/word_lookup_c... ui/views/word_lookup_client.h:30: ~WordLookupClient() {} On 2016/10/06 01:07:05, karandeepb wrote: > On 2016/10/05 06:20:55, tapted wrote: > > declare virtual? (or can it be omitted whilst there are no members?) > > Declared virtual. I think if we omit the destructor, it would be public and > non-virtual. > > Also, just for my own understanding, is declaring it virtual wrong, if I don't > expect the instance to be deleted through a base class pointer and if there is > no "delete this" in the base class implementation. Hypothetically, I think a future subclass we don't know about yet could still invoke it incorrectly. But this is one of those often-debated things that don't have a clear answer, so we defer to consistency -- "whatever most of Chrome already does in this case". (then I started looking around for some C++1?ish way to delete a destructor implementation we know is going to be trivial [without breaking polymorphism] - i.e. if something *did* invoke the base class destructor external to the class hierarchy then just crash - but that doesn't seem to be a thing).
https://codereview.chromium.org/2348143003/diff/660001/ui/views/word_lookup_c... File ui/views/word_lookup_client.h (right): https://codereview.chromium.org/2348143003/diff/660001/ui/views/word_lookup_c... ui/views/word_lookup_client.h:30: ~WordLookupClient() {} On 2016/10/06 02:03:19, tapted wrote: > On 2016/10/06 01:07:05, karandeepb wrote: > > On 2016/10/05 06:20:55, tapted wrote: > > > declare virtual? (or can it be omitted whilst there are no members?) > > > > Declared virtual. I think if we omit the destructor, it would be public and > > non-virtual. > > > > Also, just for my own understanding, is declaring it virtual wrong, if I don't > > expect the instance to be deleted through a base class pointer and if there > is > > no "delete this" in the base class implementation. > > Hypothetically, I think a future subclass we don't know about yet could still > invoke it incorrectly. But this is one of those often-debated things that don't > have a clear answer, so we defer to consistency -- "whatever most of Chrome > already does in this case". > > (then I started looking around for some C++1?ish way to delete a destructor > implementation we know is going to be trivial [without breaking polymorphism] - > i.e. if something *did* invoke the base class destructor external to the class > hierarchy then just crash - but that doesn't seem to be a thing). Makes sense, thanks!
The CQ bit was checked by karandeepb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2348143003/#ps700001 (title: "Address nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== MacViews: Implement Force Touch/Mac dictionary lookup for Textfields. This CL adds support to MacViews for Force touch and dictionary lookups. This is achieved by implementing quickLookWithEvent: on the BridgedContentView. A new interface called WordLookupClient is added which is to be implemented by views which support word lookups. Currently only views::Textfield implements this interface. A virtual method GetWordLookupClient is added to views::View to retrieve the WordLookupClient associated with a given View. Support is added to the RenderText class to retreive the word displayed at a given point along with its styling information. A DecoratedText class is also introduced to encapsulate the styling information for some given text. Note, this CL only adds support for dictionary lookups to Views::Textfields. Doc- https://docs.google.com/a/google.com/document/d/17QYrMpIXpk-FpGanjhJ-Yx33njNQ.... BUG=640502 TEST= Enable chrome://flags/#mac-views-native-dialogs. Open Bookmark Bubble. Enter some text in the name textfield. Hover over the text and press Ctrl+Command+D. Verify that a dictionary popup appears showing the definition of the word, the mouse was hovering on. ========== to ========== MacViews: Implement Force Touch/Mac dictionary lookup for Textfields. This CL adds support to MacViews for Force touch and dictionary lookups. This is achieved by implementing quickLookWithEvent: on the BridgedContentView. A new interface called WordLookupClient is added which is to be implemented by views which support word lookups. Currently only views::Textfield implements this interface. A virtual method GetWordLookupClient is added to views::View to retrieve the WordLookupClient associated with a given View. Support is added to the RenderText class to retreive the word displayed at a given point along with its styling information. A DecoratedText class is also introduced to encapsulate the styling information for some given text. Note, this CL only adds support for dictionary lookups to Views::Textfields. Doc- https://docs.google.com/a/google.com/document/d/17QYrMpIXpk-FpGanjhJ-Yx33njNQ.... BUG=640502 TEST= Enable chrome://flags/#mac-views-native-dialogs. Open Bookmark Bubble. Enter some text in the name textfield. Hover over the text and press Ctrl+Command+D. Verify that a dictionary popup appears showing the definition of the word, the mouse was hovering on. ==========
Message was sent while issue was closed.
Committed patchset #20 (id:700001)
Message was sent while issue was closed.
Description was changed from ========== MacViews: Implement Force Touch/Mac dictionary lookup for Textfields. This CL adds support to MacViews for Force touch and dictionary lookups. This is achieved by implementing quickLookWithEvent: on the BridgedContentView. A new interface called WordLookupClient is added which is to be implemented by views which support word lookups. Currently only views::Textfield implements this interface. A virtual method GetWordLookupClient is added to views::View to retrieve the WordLookupClient associated with a given View. Support is added to the RenderText class to retreive the word displayed at a given point along with its styling information. A DecoratedText class is also introduced to encapsulate the styling information for some given text. Note, this CL only adds support for dictionary lookups to Views::Textfields. Doc- https://docs.google.com/a/google.com/document/d/17QYrMpIXpk-FpGanjhJ-Yx33njNQ.... BUG=640502 TEST= Enable chrome://flags/#mac-views-native-dialogs. Open Bookmark Bubble. Enter some text in the name textfield. Hover over the text and press Ctrl+Command+D. Verify that a dictionary popup appears showing the definition of the word, the mouse was hovering on. ========== to ========== MacViews: Implement Force Touch/Mac dictionary lookup for Textfields. This CL adds support to MacViews for Force touch and dictionary lookups. This is achieved by implementing quickLookWithEvent: on the BridgedContentView. A new interface called WordLookupClient is added which is to be implemented by views which support word lookups. Currently only views::Textfield implements this interface. A virtual method GetWordLookupClient is added to views::View to retrieve the WordLookupClient associated with a given View. Support is added to the RenderText class to retreive the word displayed at a given point along with its styling information. A DecoratedText class is also introduced to encapsulate the styling information for some given text. Note, this CL only adds support for dictionary lookups to Views::Textfields. Doc- https://docs.google.com/a/google.com/document/d/17QYrMpIXpk-FpGanjhJ-Yx33njNQ.... BUG=640502 TEST= Enable chrome://flags/#mac-views-native-dialogs. Open Bookmark Bubble. Enter some text in the name textfield. Hover over the text and press Ctrl+Command+D. Verify that a dictionary popup appears showing the definition of the word, the mouse was hovering on. Committed: https://crrev.com/a56ce7b0a7e774caebf3a4dd774d6d5d7d9b8d2a Cr-Commit-Position: refs/heads/master@{#423419} ==========
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/a56ce7b0a7e774caebf3a4dd774d6d5d7d9b8d2a Cr-Commit-Position: refs/heads/master@{#423419} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
