|
|
Description[blink/mac] Support getting baseline point for showing dictionary lookup bubble.
TBR=jochen@chromium.org
BUG=121917
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=202504
Patch Set 1 #Patch Set 2 : Avoid breakge. #Patch Set 3 : #
Total comments: 4
Patch Set 4 : nit. #
Total comments: 4
Patch Set 5 : nits. #Patch Set 6 : avoid |const NSClass*| #Messages
Total messages: 26 (8 generated)
shuchen@chromium.org changed reviewers: + jochen@chromium.org
jochen@, can you please help to review this cl? Thanks
On 2015/09/09 04:46:20, Shu Chen wrote: > jochen@, can you please help to review this cl? Thanks Btw, the newly added function will be used in this cl: https://codereview.chromium.org/1318483007.
jochen@chromium.org changed reviewers: + eae@chromium.org, esprehn@chromium.org
sorry, I don't know enough about this code to review it. Elliot or Emil, is this something you could review?
https://codereview.chromium.org/1329103002/diff/40001/Source/web/mac/WebSubst... File Source/web/mac/WebSubstringUtil.mm (right): https://codereview.chromium.org/1329103002/diff/40001/Source/web/mac/WebSubst... Source/web/mac/WebSubstringUtil.mm:116: // TODO(yosin) We shold avoid to create |Range| object. Could you please file a bug about this and add a link here? https://codereview.chromium.org/1329103002/diff/40001/Source/web/mac/WebSubst... Source/web/mac/WebSubstringUtil.mm:119: stringPoint.setY(frameView->height() - stringPoint.y()); You take the bottom left corner of the bounding rect for the range and then adjust it by the height of the frame view. What type of coordinate does that give you? I know this code was just moved but an explanation here would help.
https://codereview.chromium.org/1329103002/diff/40001/Source/web/mac/WebSubst... File Source/web/mac/WebSubstringUtil.mm (right): https://codereview.chromium.org/1329103002/diff/40001/Source/web/mac/WebSubst... Source/web/mac/WebSubstringUtil.mm:116: // TODO(yosin) We shold avoid to create |Range| object. On 2015/09/09 16:56:04, eae wrote: > Could you please file a bug about this and add a link here? Done. https://codereview.chromium.org/1329103002/diff/40001/Source/web/mac/WebSubst... Source/web/mac/WebSubstringUtil.mm:119: stringPoint.setY(frameView->height() - stringPoint.y()); On 2015/09/09 16:56:04, eae wrote: > You take the bottom left corner of the bounding rect for the range and then > adjust it by the height of the frame view. What type of coordinate does that > give you? I know this code was just moved but an explanation here would help. Actually, I don't understand why adjust y() by the height of the frame view. The y() will be adjusted back when using it at here: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re.... The point type is changed as IntPoint -> WebPoint -> gfx::Point -> CGPoint -> NSPoint. I suspect the computation on certain point type requires that strange adjust. Do you have any ideas?
Why does this take a pointer but the other one take a reference? I'd be nice if this was consistent. Also this seems like a strange API to expose out of blink. Should this be in base? To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On 2015/09/10 03:01:48, esprehn wrote: > Why does this take a pointer but the other one take a reference? I'd be > nice if this was consistent. > > Also this seems like a strange API to expose out of blink. Should this be > in base? I am not familiar with blink, so I try to minimize the change as much as possible. So I am not sure whether it is legal to "expose out of blink" in non-base. For the reference/point issue, the purpose is to share the code and avoid breakage. attributedSubstringInRange() is called in content::TextInputClientObserver. So the method definition need to remain the same to avoid breakage. However, we need to support giving out the baseline point from range. To share the code with the original attributedSubstringInRange() method, I made the parameter as pointer so that the new attributedSubstringInRange method can simply pass nil. This cl cannot just change the other method attributedWordAtPoint() to use pointer parameter to avoid breakage. Please refer to cl https://codereview.chromium.org/1318483007 for the usage of this blink change.
On 2015/09/10 03:23:28, Shu Chen wrote: > On 2015/09/10 03:01:48, esprehn wrote: > > Why does this take a pointer but the other one take a reference? I'd be > > nice if this was consistent. > > > > Also this seems like a strange API to expose out of blink. Should this be > > in base? > > I am not familiar with blink, so I try to minimize the change as much as > possible. > So I am not sure whether it is legal to "expose out of blink" in non-base. > > For the reference/point issue, the purpose is to share the code and avoid > breakage. > attributedSubstringInRange() is called in content::TextInputClientObserver. > So the method definition need to remain the same to avoid breakage. > However, we need to support giving out the baseline point from range. > To share the code with the original attributedSubstringInRange() method, I made > the > parameter as pointer so that the new attributedSubstringInRange method can > simply pass nil. > > This cl cannot just change the other method attributedWordAtPoint() to use > pointer parameter to avoid breakage. > > Please refer to cl https://codereview.chromium.org/1318483007 for the usage of > this blink change. Btw, being pointer for |baselinePoint| is a temporary solution. Eventually, there should be only one attributedSubstringInRange() method, and the |baselinePoint| param should be in reference type.
Pinging...
shuchen@chromium.org changed reviewers: - jochen@chromium.org
Pinging for reivew. eae@/esprehn@, mind taking another look? Thanks
LGTM but please allow Elliot to comment before landing.
lgtm w/ nits. https://codereview.chromium.org/1329103002/diff/60001/Source/web/mac/WebSubst... File Source/web/mac/WebSubstringUtil.mm (right): https://codereview.chromium.org/1329103002/diff/60001/Source/web/mac/WebSubst... Source/web/mac/WebSubstringUtil.mm:125: if (font) We normally write this as a single statement. if (NSFont* font = [attributes objectForKey:NSFontAttributeName]) stringPoint.move(0, ceil(-[font descender])); https://codereview.chromium.org/1329103002/diff/60001/Source/web/mac/WebSubst... Source/web/mac/WebSubstringUtil.mm:172: NSAttributedString* ret = attributedSubstringFromRange(ephemeralRange); don't abbreviate. NSAttributedString* result = ...;
Thanks for you review! https://codereview.chromium.org/1329103002/diff/60001/Source/web/mac/WebSubst... File Source/web/mac/WebSubstringUtil.mm (right): https://codereview.chromium.org/1329103002/diff/60001/Source/web/mac/WebSubst... Source/web/mac/WebSubstringUtil.mm:125: if (font) On 2015/09/18 02:34:00, esprehn wrote: > We normally write this as a single statement. > > if (NSFont* font = [attributes objectForKey:NSFontAttributeName]) > stringPoint.move(0, ceil(-[font descender])); Done. https://codereview.chromium.org/1329103002/diff/60001/Source/web/mac/WebSubst... Source/web/mac/WebSubstringUtil.mm:172: NSAttributedString* ret = attributedSubstringFromRange(ephemeralRange); On 2015/09/18 02:34:00, esprehn wrote: > don't abbreviate. > > NSAttributedString* result = ...; Done.
The CQ bit was checked by shuchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org, eae@chromium.org Link to the patchset: https://codereview.chromium.org/1329103002/#ps80001 (title: "nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1329103002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1329103002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
TBR'ing jochen@ for rubber stamp on WebSubstringUtil.h.
The CQ bit was checked by shuchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org, eae@chromium.org Link to the patchset: https://codereview.chromium.org/1329103002/#ps100001 (title: "avoid |const NSClass*|")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1329103002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1329103002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://src.chromium.org/viewvc/blink?view=rev&revision=202504 |