Issue 2278283002: Implement Mac Pop-up Dictionary for OOPIF.

EhsanK

The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run

4 years, 3 months ago (2016-08-25 20:55:07 UTC) #1

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2278283002/1

4 years, 3 months ago (2016-08-25 20:55:45 UTC) #2

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 3 months ago (2016-08-25 21:06:55 UTC) #3

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_android/builds/118569) cast_shell_linux on ...

4 years, 3 months ago (2016-08-25 21:06:56 UTC) #4

EhsanK

The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run

4 years, 3 months ago (2016-08-25 21:55:33 UTC) #5

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2278283002/1

4 years, 3 months ago (2016-08-25 21:56:06 UTC) #6

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 3 months ago (2016-08-25 22:08:58 UTC) #7

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-generic_chromium_compile_only_ng/builds/189939)

4 years, 3 months ago (2016-08-25 22:08:58 UTC) #8

EhsanK

The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run

4 years, 3 months ago (2016-08-25 22:13:21 UTC) #9

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2278283002/20001

4 years, 3 months ago (2016-08-25 22:13:57 UTC) #10

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 3 months ago (2016-08-25 22:37:59 UTC) #11

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/285106)

4 years, 3 months ago (2016-08-25 22:38:00 UTC) #12

EhsanK

Description was changed from ========== Implement the logic to support pop-up Dictionary on OOPIF (Mac) ...

4 years, 3 months ago (2016-08-30 14:55:58 UTC) #13

EhsanK

The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run

4 years, 3 months ago (2016-08-30 16:46:04 UTC) #14

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2278283002/80001

4 years, 3 months ago (2016-08-30 16:46:23 UTC) #15

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 3 months ago (2016-08-30 17:03:32 UTC) #16

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/287440)

4 years, 3 months ago (2016-08-30 17:03:33 UTC) #17

EhsanK

The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run

4 years, 3 months ago (2016-08-30 18:55:46 UTC) #18

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2278283002/100001

4 years, 3 months ago (2016-08-30 18:56:37 UTC) #19

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 3 months ago (2016-08-30 21:16:51 UTC) #20

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/284273)

4 years, 3 months ago (2016-08-30 21:16:52 UTC) #21

EhsanK

ekaramad@chromium.org changed reviewers: + creis@chromium.org, kenrb@chromium.org

4 years, 3 months ago (2016-08-31 15:07:34 UTC) #22

EhsanK

kenrb@, creis: Could you please review this patch? I will add Mac and blink reviews ...

4 years, 3 months ago (2016-08-31 15:07:34 UTC) #23

Charlie Reis

On 2016/08/31 15:07:34, EhsanK wrote: > kenrb@, creis: Could you please review this patch? I ...

4 years, 3 months ago (2016-08-31 19:40:37 UTC) #24

EhsanK

ekaramad@chromium.org changed reviewers: + wjmaclean@chromium.org

4 years, 3 months ago (2016-08-31 20:20:49 UTC) #25

EhsanK

Thanks Charlie. Maybe James could review the RWHIER part? wjmaclean@ Could you please review this ...

4 years, 3 months ago (2016-08-31 20:20:50 UTC) #26

wjmaclean

On 2016/08/31 20:20:50, EhsanK wrote: > Thanks Charlie. Maybe James could review the RWHIER part? ...

4 years, 3 months ago (2016-08-31 20:30:58 UTC) #27

EhsanK

Thanks James! I think there might have been no need of finding out the Widget ...

4 years, 3 months ago (2016-08-31 21:59:49 UTC) #28

Charlie Reis

creis@chromium.org changed reviewers: + erikchen@chromium.org, lfg@chromium.org

4 years, 3 months ago (2016-08-31 22:50:01 UTC) #29

Charlie Reis

Actually adding erikchen per Ehsan's previous message. Also +lfg for a question below, and for ...

4 years, 3 months ago (2016-08-31 22:50:03 UTC) #30

erikchen

https://codereview.chromium.org/2278283002/diff/100001/content/browser/renderer_host/render_widget_host_view_mac.mm File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/2278283002/diff/100001/content/browser/renderer_host/render_widget_host_view_mac.mm#newcode2311 content/browser/renderer_host/render_widget_host_view_mac.mm:2311: TextInputClientMac::GetInstance()->GetStringAtPoint( It looks like if this method is called ...

4 years, 3 months ago (2016-09-01 00:04:29 UTC) #31

lfg

Also adding avi@ since I discussed plugins and RenderWidgets with him in a previous CL. ...

4 years, 3 months ago (2016-09-01 15:42:28 UTC) #33

EhsanK

The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run

4 years, 3 months ago (2016-09-01 20:43:17 UTC) #35

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2278283002/160001

4 years, 3 months ago (2016-09-01 20:44:13 UTC) #36

EhsanK

PTAL. https://codereview.chromium.org/2278283002/diff/100001/content/browser/renderer_host/render_widget_host_input_event_router.h File content/browser/renderer_host/render_widget_host_input_event_router.h (right): https://codereview.chromium.org/2278283002/diff/100001/content/browser/renderer_host/render_widget_host_input_event_router.h#newcode84 content/browser/renderer_host/render_widget_host_input_event_router.h:84: RenderWidgetHost* GetRenderWidgetHostAtPos( On 2016/08/31 22:50:03, Charlie Reis wrote: ...

4 years, 3 months ago (2016-09-01 21:58:37 UTC) #37

PTAL.

https://codereview.chromium.org/2278283002/diff/100001/content/browser/render...
File content/browser/renderer_host/render_widget_host_input_event_router.h
(right):

https://codereview.chromium.org/2278283002/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_input_event_router.h:84:
RenderWidgetHost* GetRenderWidgetHostAtPos(
On 2016/08/31 22:50:03, Charlie Reis wrote:
> nit: Needs comment (e.g., explaining |point| vs |transformed_point|).

Done.

https://codereview.chromium.org/2278283002/diff/100001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_mac.h (right):

https://codereview.chromium.org/2278283002/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac.h:478: // Returns the
RenderWidgetHost returned form hit testing at the given point
On 2016/08/31 22:50:03, Charlie Reis wrote:
> s/form/from/?

Done.

https://codereview.chromium.org/2278283002/diff/100001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/2278283002/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac.mm:558:
RenderWidgetHost* RenderWidgetHostViewMac::GetRenderWidgetHostAtPoint(
On 2016/08/31 22:50:03, Charlie Reis wrote:
> Why not RenderWidgetHostImpl, as above?  We're within content, so might as
well
> avoid a future need for static casts.

I guess I just put what I needed. But I agree with the future use argument.
Thanks! Done!

https://codereview.chromium.org/2278283002/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac.mm:2311:
TextInputClientMac::GetInstance()->GetStringAtPoint(
On 2016/09/01 00:04:29, erikchen wrote:
> It looks like if this method is called twice in quick succession (which looks
> possible, since this is triggered by a cocoa event), then TextInputClientMac
> will use the second callback with the first response from the renderer. Seems
> like a bug? [I know that it was already present, and not related to your
> changes. Even if you don't fix it, could you add a comment and file a bug?]

We actually fire a DCHECK in that scenario. I added some comments below but I
think it might be wise to have a mechanism of tracking callbacks per RWH (i.e.,
map<process_id, map<routing_id, callback>> or somehing).

I am leaving a TODO in the TextInputClientMac class for this purpose (also filed
a bug).

https://codereview.chromium.org/2278283002/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac.mm:2317: [self
showLookUpDictionaryOverlayInternal:string
On 2016/09/01 00:04:28, erikchen wrote:
> Is it ever possible for this callback to not be invoked (e.g. renderer process
> dies at inopportune time?) If so, it looks like this block will be leaked,
which
> will leak a strong reference to "self", which could cause a retain cycle.
> 
> Perhaps this should be a weak reference using __weak
> (RenderWidgetHostViewCocoa*) weakSelf = self?

Yes it is - as far as I can tell - possible to have this callback not invoked.
We only clear the call back on a new request (which we also DCHECK if there is
an existing one), or when we receive the IPC back. There is also no 
logic what so ever to clear the callback in TextInputClientMac when a RWHVCocoa
is going away or something. 

But I am not sure if there will be a retain cycle as the RWHVCocoa is not
keeping this callback object (TextInputClientMac is which is a singleton). 

But that being said, the DCHECK inside TextInputClientMac for an existing
handler when we make a new request (check fail if there is an existing one). I
am not quite convinced with the design and I think it might need to changes. To
crash the DCHECK all you need to do is to search for a word in a janky page and
then switch tabs and search for another word. This issue is too easily
reproducible IMO for a DCHECK.

Maybe we need a map<RWH, callback> and clear those callbacks when deemed
necessary?

https://codereview.chromium.org/2278283002/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac.mm:2839:
gfx::Point(thePoint.x, thePoint.y), &transformedPoint);
On 2016/09/01 00:04:29, erikchen wrote:
> Doesn't this need a nullptr check?

There is two cases that we might dereference a null here and inside the method
GetRenderWidgetHostAtPoint().

1) If RenderWidgetHostInputEventRouter returns null for the WidgetAtPoint, which
I think is unlikely. wjmaclean@ might have a better answer to that.

2) If we destroy the RenderWidgetHost for a child frame, the
RenderWidgetHostViewChildFrame is not destroyed immediately, which will still be
registered as a viable view inside RenderWidgetHostInputEventRouter. So we might
endup with a valid pointer to RWHVCF when we call
RenderWidgetHostInputEventRouter::FindEventTarget but then the view will have a
null RenderWidgetHost.

Separate note: while investigating this I noticed NSTextInput is deprecated past
10.6. (I guess bug 479010?). There are several methods including this one here
to remove. Just curious if there is any progress/plans towards that.

https://codereview.chromium.org/2278283002/diff/100001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2278283002/diff/100001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:7942: base::Unretained(this)));
On 2016/09/01 00:04:29, erikchen wrote:
> why don't you need to run the RunLoop here? If that's a mistake, then the test
> shouldn't be passing, right?
> 
> It looks like you want to reset helper.word() before reusing the helper (or
just
> make a new helper). Maybe WaitForString* should reset word(), to prevent this
> type of error from happening?

I should have used the loop. Fails without it since range (0, 5) is the word
"This " and what we get from GetStrignAtPoint is "This". So I fixed the range to
be 0 - 4 and now the test is happy (and correct).

With the message loop, helper.word() and helper.point() will get reset to the
new value after the loop stops (if it doesn't test times out). So I don't think
I should reset them now.

https://codereview.chromium.org/2278283002/diff/100001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:7970: void
SetDocumentBodyHTML(FrameTreeNode* node, const std::string& html) {
On 2016/09/01 00:04:29, erikchen wrote:
> This should be in an anonymous namespace?
Actually I do not need this method anymore. Sorry for leaving it there. Removed.

https://codereview.chromium.org/2278283002/diff/100001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:7981: // the
On 2016/08/31 22:50:03, Charlie Reis wrote:
> nit: Fix line wrap.

Done. When I get a chance I will take a look at 'git cl format' code for
wrapping comments. It does not do a good job when you extend only one line of
comment to over 80 and then call it.

https://codereview.chromium.org/2278283002/diff/100001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:8001: std::string word =
helper.word();
On 2016/09/01 00:04:29, erikchen wrote:
> can you add an expectation that verifies the contents of "word"? ditto on the
> next test.

Done. Thanks! (the word to be verified is "This").

https://codereview.chromium.org/2278283002/diff/100001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:8036: #endif
On 2016/09/01 00:04:29, erikchen wrote:
> s/#endif/#endif  // defined(OS_MACOSX)

Done.

https://codereview.chromium.org/2278283002/diff/100001/content/renderer/rende...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/2278283002/diff/100001/content/renderer/rende...
content/renderer/render_widget.cc:471: OnTextInputClientMessageReceived(this,
message));
On 2016/08/31 22:50:03, Charlie Reis wrote:
> Why is this needed?  (Forwarding IPCs can be risky, vs routing them to the
right
> place to begin with.)  Are we supposed to continue below afterward, or return
> early?

All such IPCs are triggered by RenderWidgetHostViewCocoa and sent through
RenderWidgetHost. I see two options here:
Option-1) Send the IPC to RenderWidget (as it is right now) and handle it here.
Unfortunately, for reasons such as accessing focused plugin and also
focusedFrame we cannot have RenderWidget handle this now. The issues are
 1-1) WebViewImpl is a WebWidget but inheritance is protected. So later in
TextInputClientObserver we cannot down cast to WebViewImpl. Right now I am
passing both WebWidget and WebView to WebSubstringUtil methods which is ugly as
well.
  1-2) We need focused plugin. This could be resolved though if we don't have
issues with 1-1). I can search the frame tree and cast frames to RenderFrame the
same way RenderViewImpl::GetFocusedPepperPlugin() works now.

Resolving 1-1) and 1-2) will enable us to move TextInputClientObserver to
RenderWidget and avoid this observer call (handle IPC right here).

Option-2) Send the IPC to RenderFrameImpl. But this will be uglier (IMO) on the
browser side. We will have to get from RenderWidgetHost to RenderFrame to send
the IPC. Also, some parts of TextInputClientMac relate to IME (dictionary pop-up
is some sort of input I guess).
My suggestion is proceeding with Option-1 until we can give the ownership of
TextInputClientObserver to RenderWidget. Maybe this becomes a possibility (minus
the plugin access problem) when Lucas's patch lands:
https://codereview.chromium.org/2288313002/

https://codereview.chromium.org/2278283002/diff/100001/content/renderer/rende...
File content/renderer/render_widget.h (right):

https://codereview.chromium.org/2278283002/diff/100001/content/renderer/rende...
content/renderer/render_widget.h:383: PepperPluginInstanceImpl*
focused_pepper_plugin() const {
On 2016/09/01 15:42:27, lfg wrote:
> On 2016/08/31 22:50:03, Charlie Reis wrote:
> > lfg@: Can you review this part?  I know you had some thoughts on this
> recently.
> 
> I really don't like this. We shouldn't be exposing focused_pepper_plugin()
> through RenderWidget. I was ok with adding this to RenderWidget because we had
> to fix the blocking issue with IME, but I think we should now try to find a
> better solution.
> 
> Here's what I propose: 1) Add a RenderWidgetDelegate abstraction that has the
> needed IME-related functions. 2) Add an IME class the implements
> RenderWidgetDelegate. This IME class can keep track of all the frames in the
> local root, and we should rely on the RenderFrame as the only source for the
> focused_pepper_plugin (we already saw that having this in RenderWidget is
> fragile with a use-after-free last week).

For now I have removed this line of code and pass RenderFrameImpl (local root)
to TextInputClientObserver).

I think having a middle interface like the RenderWidgetDelegate is very useful.
I am not sure if RenderFrameImpl is a good candidate for implementing the
interface, or we should have a separate implementation owned by (maybe)
RenderFrame.

https://codereview.chromium.org/2278283002/diff/100001/content/renderer/text_...
File content/renderer/text_input_client_observer.h (right):

https://codereview.chromium.org/2278283002/diff/100001/content/renderer/text_...
content/renderer/text_input_client_observer.h:29: blink::WebView* webv_iew);
On 2016/09/01 00:04:29, erikchen wrote:
> s/webv_iew/web_view

Thanks. Changed the constructors signature and this field is removed now.

https://codereview.chromium.org/2278283002/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/web/mac/WebSubstringUtil.h (right):

https://codereview.chromium.org/2278283002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/web/mac/WebSubstringUtil.h:51: // TODO(ekaramad): This
method should only receive a WebWidget. But due to
On 2016/09/01 00:04:29, erikchen wrote:
> Is there a typo in this comment? I think one of the instances of WebWidget
> should be changed to WebView?

It is correct but vague perhaps. I reworded this. Thanks!

Ideally, we want only WebWidget here. The reason is that this operation should
be handled by RenderWidget (the IPC is sent to RenderWidget). But inside
RenderWidget we only have a WebWidget reference.  Although WebViewImpl is a
WebWidget, but the inheritance is protected so here I cannot rely on only
WebWidget reference to handle the WebViewImpl case (for the main frame).

erikchen

lgtm % discussion with lfg@ about plugins and render widgets. https://codereview.chromium.org/2278283002/diff/100001/content/browser/renderer_host/render_widget_host_view_mac.mm File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/2278283002/diff/100001/content/browser/renderer_host/render_widget_host_view_mac.mm#newcode2317 ...

4 years, 3 months ago (2016-09-02 01:10:39 UTC) #38

lgtm % discussion with lfg@ about plugins and render widgets.

https://codereview.chromium.org/2278283002/diff/100001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/2278283002/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac.mm:2317: [self
showLookUpDictionaryOverlayInternal:string
On 2016/09/01 21:58:36, EhsanK wrote:
> On 2016/09/01 00:04:28, erikchen wrote:
> > Is it ever possible for this callback to not be invoked (e.g. renderer
process
> > dies at inopportune time?) If so, it looks like this block will be leaked,
> which
> > will leak a strong reference to "self", which could cause a retain cycle.
> > 
> > Perhaps this should be a weak reference using __weak
> > (RenderWidgetHostViewCocoa*) weakSelf = self?
> 
> Yes it is - as far as I can tell - possible to have this callback not invoked.
> We only clear the call back on a new request (which we also DCHECK if there is
> an existing one), or when we receive the IPC back. There is also no 
> logic what so ever to clear the callback in TextInputClientMac when a
RWHVCocoa
> is going away or something. 
> 
> But I am not sure if there will be a retain cycle as the RWHVCocoa is not
> keeping this callback object (TextInputClientMac is which is a singleton). 
> 
> But that being said, the DCHECK inside TextInputClientMac for an existing
> handler when we make a new request (check fail if there is an existing one). I
> am not quite convinced with the design and I think it might need to changes.
To
> crash the DCHECK all you need to do is to search for a word in a janky page
and
> then switch tabs and search for another word. This issue is too easily
> reproducible IMO for a DCHECK.
> 
> Maybe we need a map<RWH, callback> and clear those callbacks when deemed
> necessary? 

That would be good. Doesn't have to be in this CL, since the problem was already
present. Note that a DCHECK should never trigger. If it triggers, it means
something is wrong with the code (or dcheck). See
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

https://codereview.chromium.org/2278283002/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac.mm:2839:
gfx::Point(thePoint.x, thePoint.y), &transformedPoint);
On 2016/09/01 21:58:36, EhsanK wrote:
> On 2016/09/01 00:04:29, erikchen wrote:
> > Doesn't this need a nullptr check?
> 
> There is two cases that we might dereference a null here and inside the method
> GetRenderWidgetHostAtPoint().
> 
> 1) If RenderWidgetHostInputEventRouter returns null for the WidgetAtPoint,
which
> I think is unlikely. wjmaclean@ might have a better answer to that.
> 
> 2) If we destroy the RenderWidgetHost for a child frame, the
> RenderWidgetHostViewChildFrame is not destroyed immediately, which will still
be
> registered as a viable view inside RenderWidgetHostInputEventRouter. So we
might
> endup with a valid pointer to RWHVCF when we call
> RenderWidgetHostInputEventRouter::FindEventTarget but then the view will have
a
> null RenderWidgetHost.
> 
> Separate note: while investigating this I noticed NSTextInput is deprecated
past
> 10.6. (I guess bug 479010?). There are several methods including this one here
> to remove. Just curious if there is any progress/plans towards that.

Thanks for looking into this. With regards to NSTextInput
deprecation...eventually, hopefully? Unfortunately it's not a high priority
right now.

lfg

https://codereview.chromium.org/2278283002/diff/100001/content/renderer/render_widget.h File content/renderer/render_widget.h (right): https://codereview.chromium.org/2278283002/diff/100001/content/renderer/render_widget.h#newcode383 content/renderer/render_widget.h:383: PepperPluginInstanceImpl* focused_pepper_plugin() const { On 2016/09/01 21:58:37, EhsanK wrote: ...

4 years, 3 months ago (2016-09-02 16:39:00 UTC) #39

Charlie Reis

Thanks. Another round of cleanup comments below. https://codereview.chromium.org/2278283002/diff/100001/content/renderer/render_widget.cc File content/renderer/render_widget.cc (right): https://codereview.chromium.org/2278283002/diff/100001/content/renderer/render_widget.cc#newcode471 content/renderer/render_widget.cc:471: OnTextInputClientMessageReceived(this, message)); ...

4 years, 3 months ago (2016-09-02 21:38:17 UTC) #41

Thanks.  Another round of cleanup comments below.

https://codereview.chromium.org/2278283002/diff/100001/content/renderer/rende...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/2278283002/diff/100001/content/renderer/rende...
content/renderer/render_widget.cc:471: OnTextInputClientMessageReceived(this,
message));
On 2016/09/01 21:58:37, EhsanK wrote:
> On 2016/08/31 22:50:03, Charlie Reis wrote:
> > Why is this needed?  (Forwarding IPCs can be risky, vs routing them to the
> right
> > place to begin with.)  Are we supposed to continue below afterward, or
return
> > early?
> 
> All such IPCs are triggered by RenderWidgetHostViewCocoa and sent through
> RenderWidgetHost. I see two options here:
> Option-1) Send the IPC to RenderWidget (as it is right now) and handle it
here.
> Unfortunately, for reasons such as accessing focused plugin and also
> focusedFrame we cannot have RenderWidget handle this now. The issues are
>  1-1) WebViewImpl is a WebWidget but inheritance is protected. So later in
> TextInputClientObserver we cannot down cast to WebViewImpl. Right now I am
> passing both WebWidget and WebView to WebSubstringUtil methods which is ugly
as
> well.
>   1-2) We need focused plugin. This could be resolved though if we don't have
> issues with 1-1). I can search the frame tree and cast frames to RenderFrame
the
> same way RenderViewImpl::GetFocusedPepperPlugin() works now.
> 
> Resolving 1-1) and 1-2) will enable us to move TextInputClientObserver to
> RenderWidget and avoid this observer call (handle IPC right here).
> 
> Option-2) Send the IPC to RenderFrameImpl. But this will be uglier (IMO) on
the
> browser side. We will have to get from RenderWidgetHost to RenderFrame to send
> the IPC. Also, some parts of TextInputClientMac relate to IME (dictionary
pop-up
> is some sort of input I guess).
> My suggestion is proceeding with Option-1 until we can give the ownership of
> TextInputClientObserver to RenderWidget. Maybe this becomes a possibility
(minus
> the plugin access problem) when Lucas's patch lands:
> https://codereview.chromium.org/2288313002/

Hmm.  If the intention is to move TextInputClientObserver to RenderWidget, then
I suppose we can keep having the IPCs sent here.

How many messages are we talking about?  I'm wondering if it's clearer to have
IPC handlers for them here in RenderWidget and just have them call a
corresponding method on the local root RenderFrame for now (which passes it on
to the TextInputClientObserver).

If there's several messages, then I suppose we can keep this approach but add a
comment explaining why we have it.  If we do stick with this approach, is there
a way to send it to just the local root RenderFrame and not all RenderFrames of
the widget?  And shouldn't we be returning early?

Charlie Reis

Wait, I had a lot of comments. Did you delete a patch set? Charlie On ...

4 years, 3 months ago (2016-09-02 21:39:17 UTC) #42

Wait, I had a lot of comments.  Did you delete a patch set?

Charlie

On Fri, Sep 2, 2016 at 2:38 PM, <creis@chromium.org> wrote:

> Thanks. Another round of cleanup comments below.
>
>
> https://codereview.chromium.org/2278283002/diff/100001/
> content/renderer/render_widget.cc
> File content/renderer/render_widget.cc (right):
>
> https://codereview.chromium.org/2278283002/diff/100001/
> content/renderer/render_widget.cc#newcode471
> content/renderer/render_widget.cc:471:
> OnTextInputClientMessageReceived(this, message));
> On 2016/09/01 21:58:37, EhsanK wrote:
> > On 2016/08/31 22:50:03, Charlie Reis wrote:
> > > Why is this needed? (Forwarding IPCs can be risky, vs routing them
> to the
> > right
> > > place to begin with.) Are we supposed to continue below afterward,
> or return
> > > early?
> >
> > All such IPCs are triggered by RenderWidgetHostViewCocoa and sent
> through
> > RenderWidgetHost. I see two options here:
> > Option-1) Send the IPC to RenderWidget (as it is right now) and handle
> it here.
> > Unfortunately, for reasons such as accessing focused plugin and also
> > focusedFrame we cannot have RenderWidget handle this now. The issues
> are
> > 1-1) WebViewImpl is a WebWidget but inheritance is protected. So
> later in
> > TextInputClientObserver we cannot down cast to WebViewImpl. Right now
> I am
> > passing both WebWidget and WebView to WebSubstringUtil methods which
> is ugly as
> > well.
> > 1-2) We need focused plugin. This could be resolved though if we
> don't have
> > issues with 1-1). I can search the frame tree and cast frames to
> RenderFrame the
> > same way RenderViewImpl::GetFocusedPepperPlugin() works now.
> >
> > Resolving 1-1) and 1-2) will enable us to move TextInputClientObserver
> to
> > RenderWidget and avoid this observer call (handle IPC right here).
> >
> > Option-2) Send the IPC to RenderFrameImpl. But this will be uglier
> (IMO) on the
> > browser side. We will have to get from RenderWidgetHost to RenderFrame
> to send
> > the IPC. Also, some parts of TextInputClientMac relate to IME
> (dictionary pop-up
> > is some sort of input I guess).
> > My suggestion is proceeding with Option-1 until we can give the
> ownership of
> > TextInputClientObserver to RenderWidget. Maybe this becomes a
> possibility (minus
> > the plugin access problem) when Lucas's patch lands:
> > https://codereview.chromium.org/2288313002/
>
> Hmm. If the intention is to move TextInputClientObserver to
> RenderWidget, then I suppose we can keep having the IPCs sent here.
>
> How many messages are we talking about? I'm wondering if it's clearer
> to have IPC handlers for them here in RenderWidget and just have them
> call a corresponding method on the local root RenderFrame for now (which
> passes it on to the TextInputClientObserver).
>
> If there's several messages, then I suppose we can keep this approach
> but add a comment explaining why we have it. If we do stick with this
> approach, is there a way to send it to just the local root RenderFrame
> and not all RenderFrames of the widget? And shouldn't we be returning
> early?
>
> https://codereview.chromium.org/2278283002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

Charlie Reis

Wait, I had a lot of comments. Did you delete a patch set? Charlie On ...

4 years, 3 months ago (2016-09-02 21:39:19 UTC) #43

Wait, I had a lot of comments.  Did you delete a patch set?

Charlie

On Fri, Sep 2, 2016 at 2:38 PM, <creis@chromium.org> wrote:

> Thanks. Another round of cleanup comments below.
>
>
> https://codereview.chromium.org/2278283002/diff/100001/
> content/renderer/render_widget.cc
> File content/renderer/render_widget.cc (right):
>
> https://codereview.chromium.org/2278283002/diff/100001/
> content/renderer/render_widget.cc#newcode471
> content/renderer/render_widget.cc:471:
> OnTextInputClientMessageReceived(this, message));
> On 2016/09/01 21:58:37, EhsanK wrote:
> > On 2016/08/31 22:50:03, Charlie Reis wrote:
> > > Why is this needed? (Forwarding IPCs can be risky, vs routing them
> to the
> > right
> > > place to begin with.) Are we supposed to continue below afterward,
> or return
> > > early?
> >
> > All such IPCs are triggered by RenderWidgetHostViewCocoa and sent
> through
> > RenderWidgetHost. I see two options here:
> > Option-1) Send the IPC to RenderWidget (as it is right now) and handle
> it here.
> > Unfortunately, for reasons such as accessing focused plugin and also
> > focusedFrame we cannot have RenderWidget handle this now. The issues
> are
> > 1-1) WebViewImpl is a WebWidget but inheritance is protected. So
> later in
> > TextInputClientObserver we cannot down cast to WebViewImpl. Right now
> I am
> > passing both WebWidget and WebView to WebSubstringUtil methods which
> is ugly as
> > well.
> > 1-2) We need focused plugin. This could be resolved though if we
> don't have
> > issues with 1-1). I can search the frame tree and cast frames to
> RenderFrame the
> > same way RenderViewImpl::GetFocusedPepperPlugin() works now.
> >
> > Resolving 1-1) and 1-2) will enable us to move TextInputClientObserver
> to
> > RenderWidget and avoid this observer call (handle IPC right here).
> >
> > Option-2) Send the IPC to RenderFrameImpl. But this will be uglier
> (IMO) on the
> > browser side. We will have to get from RenderWidgetHost to RenderFrame
> to send
> > the IPC. Also, some parts of TextInputClientMac relate to IME
> (dictionary pop-up
> > is some sort of input I guess).
> > My suggestion is proceeding with Option-1 until we can give the
> ownership of
> > TextInputClientObserver to RenderWidget. Maybe this becomes a
> possibility (minus
> > the plugin access problem) when Lucas's patch lands:
> > https://codereview.chromium.org/2288313002/
>
> Hmm. If the intention is to move TextInputClientObserver to
> RenderWidget, then I suppose we can keep having the IPCs sent here.
>
> How many messages are we talking about? I'm wondering if it's clearer
> to have IPC handlers for them here in RenderWidget and just have them
> call a corresponding method on the local root RenderFrame for now (which
> passes it on to the TextInputClientObserver).
>
> If there's several messages, then I suppose we can keep this approach
> but add a comment explaining why we have it. If we do stick with this
> approach, is there a way to send it to just the local root RenderFrame
> and not all RenderFrames of the widget? And shouldn't we be returning
> early?
>
> https://codereview.chromium.org/2278283002/
>

-- 
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.

Charlie Reis

Ok, I think I've retyped them all. https://codereview.chromium.org/2278283002/diff/220001/content/browser/renderer_host/render_widget_host_input_event_router.cc File content/browser/renderer_host/render_widget_host_input_event_router.cc (right): https://codereview.chromium.org/2278283002/diff/220001/content/browser/renderer_host/render_widget_host_input_event_router.cc#newcode14 content/browser/renderer_host/render_widget_host_input_event_router.cc:14: #include "content/public/browser/render_widget_host.h" ...

4 years, 3 months ago (2016-09-02 21:57:48 UTC) #44

kenrb

I have a couple of comments. The temporary situation on the renderer side looks ugly ...

4 years, 3 months ago (2016-09-06 18:14:48 UTC) #45

Charlie Reis

https://codereview.chromium.org/2278283002/diff/220001/content/browser/renderer_host/render_widget_host_input_event_router.h File content/browser/renderer_host/render_widget_host_input_event_router.h (right): https://codereview.chromium.org/2278283002/diff/220001/content/browser/renderer_host/render_widget_host_input_event_router.h#newcode41 content/browser/renderer_host/render_widget_host_input_event_router.h:41: class RenderWidgetHostImpl; On 2016/09/06 18:14:48, kenrb wrote: > On ...

4 years, 3 months ago (2016-09-06 21:58:26 UTC) #46

EhsanK

Thanks Charlie and Ken for the reviews. Specially Charlie for retyping everything (sorry!). I think ...

4 years, 3 months ago (2016-09-08 17:10:40 UTC) #47

Thanks Charlie and Ken for the reviews. Specially Charlie for retyping
everything (sorry!).

I think it will be worthwhile waiting for lfg@'s patch on
RenderWidget::GetWebWidget() to land. Then we can make sure the webwidget() is
always a WebFrameWidget. With that the code in RenderFrame will go to
RenderWidget and the blink API will also not be this hacky (having both WebView
and WebWidget as arg for WebSubstringUtil::attributedWord...).

PTAL.

https://codereview.chromium.org/2278283002/diff/220001/content/browser/render...
File content/browser/renderer_host/render_widget_host_input_event_router.cc
(right):

https://codereview.chromium.org/2278283002/diff/220001/content/browser/render...
content/browser/renderer_host/render_widget_host_input_event_router.cc:14:
#include "content/public/browser/render_widget_host.h"
On 2016/09/02 21:57:47, Charlie Reis wrote:
> Stale?

Yes. Thanks!

https://codereview.chromium.org/2278283002/diff/220001/content/browser/render...
File content/browser/renderer_host/render_widget_host_input_event_router.h
(right):

https://codereview.chromium.org/2278283002/diff/220001/content/browser/render...
content/browser/renderer_host/render_widget_host_input_event_router.h:83: #if
defined(OS_MACOSX)
On 2016/09/06 18:14:48, kenrb wrote:
> Is the #ifdef necessary here? I don't see anything wrong with exposing this as
a
> method for all platforms, even if it is only currently used on one.

Acknowledged.

https://codereview.chromium.org/2278283002/diff/220001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_mac.h (right):

https://codereview.chromium.org/2278283002/diff/220001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac.h:43: class
RenderWidgetHost;
On 2016/09/02 21:57:47, Charlie Reis wrote:
> Stale?

Based on my changes, yes. But I think we should keep it as there are direct
references to RenderWidgetHost throughout the code in this file (lines ~ 258 and
~243).

https://codereview.chromium.org/2278283002/diff/220001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac.h:482:
RenderWidgetHostImpl* GetRenderWidgetHostAtPoint(
On 2016/09/06 18:14:48, kenrb wrote:
> I am not sure it is worth adding this method, rather than just having its
> callers call
>
renderWidgetHostView_->GetRenderWidgetHost()->delegate()->GetInputEventRouter()->GetRenderWidgetHostAtPoint().
> This expands the class' public API just to encapsulate some nullptr checks.

Acknowledged.

https://codereview.chromium.org/2278283002/diff/220001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/2278283002/diff/220001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac.mm:63: #include
"content/public/browser/render_widget_host.h"
On 2016/09/02 21:57:47, Charlie Reis wrote:
> Stale?  (Do we need the impl file?)

impl file is already included on line 46. I think we should keep this as IWYU
rule. But I am not using this in this patch. But generally this file makes
references to RenderWidgetHost* so I am not quite sure if we should remove it.
If so I will remove this along with all the forward decs in this file and the .h
file.

https://codereview.chromium.org/2278283002/diff/220001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac.mm:100: using
content::RenderWidgetHost;
On 2016/09/02 21:57:47, Charlie Reis wrote:
> Stale?

Same discussion as the .h file. For the override
RenderWidgetHostViewMac::GetRenderWidgetHost() we are returning a
RenderWidgetHost*.

https://codereview.chromium.org/2278283002/diff/220001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac.mm:2307:
RenderWidgetHost* widgetHost =
On 2016/09/02 21:57:47, Charlie Reis wrote:
> RenderWidgetHostImpl

Done.

https://codereview.chromium.org/2278283002/diff/220001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac.mm:2321:
root_box.bottom_left().y() - view_box.bottom_left().y();
On 2016/09/06 18:14:48, kenrb wrote:
> I don't really understand why this point gets shifted,
The return coordinate is:
1) With respect to topmost local frame (I don't think it is always local root?)
2) inverted y-axis.

So when it comes back for a child frame we have to account for these things.
Because of the inverted y-axis
issue we might not be able to use RWHV::TransformPointToRootCoordSpace.
Furthermore, the callback here
is not on UI (I assume IO). So if we want to use that method we have to thread
hop.

> and in particular why
> does the code shift a point headed to the top-level frame, even though
> previously it didn't do that?
As I understand, we get the RWH at a given point on line 2309 and at the same
time transform the point in the
root view to the RWH coordinates (right?). Then we technically transform the
point before sending the IPC and after we get the response.

https://codereview.chromium.org/2278283002/diff/220001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac.mm:2842:
RenderWidgetHost* widgetHost =
On 2016/09/02 21:57:47, Charlie Reis wrote:
> RenderWidgetHostImpl

Done.

https://codereview.chromium.org/2278283002/diff/220001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2278283002/diff/220001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:8052: class
SitePerProcessTextInputClientMacTest : public SitePerProcessBrowserTest {
On 2016/09/02 21:57:47, Charlie Reis wrote:
> We should probably put this in a new test file.  This one is already over 8000
> lines, and it's better not to mix a platform-specific section into it.  :)

Sure. Is it OK to open up a new file just for this test? I guess we might at
some point add more tests since TextInputClientMac seems to be needing some
serious fixes.

https://codereview.chromium.org/2278283002/diff/220001/content/public/test/co...
File content/public/test/content_browser_test_utils.h (right):

https://codereview.chromium.org/2278283002/diff/220001/content/public/test/co...
content/public/test/content_browser_test_utils.h:148: void
GetStringAtPointForRenderWidget(
On 2016/09/02 21:57:47, Charlie Reis wrote:
> Please document all new functions in content/public files.

Acknowledged.

https://codereview.chromium.org/2278283002/diff/220001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2278283002/diff/220001/content/renderer/rende...
content/renderer/render_frame_impl.cc:6327: // Only the local root owning the
widget should handle these messages.
On 2016/09/02 21:57:47, Charlie Reis wrote:
> This comment refers to the previous line and should come before it.

Done.

https://codereview.chromium.org/2278283002/diff/220001/content/renderer/rende...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/2278283002/diff/220001/content/renderer/rende...
content/renderer/render_frame_impl.h:1281:
std::unique_ptr<TextInputClientObserver> text_input_client_observer_;
On 2016/09/02 21:57:47, Charlie Reis wrote:
> Maybe add a TODO to move this to RenderWidget if that's the intention?

Done. I added a TODO. That being said, with lfg@'s ongoing patch on returning a
WebFrameWidgetBase 
for RenderWidget::GetWebWidget() I think I will be able to get rid of this for
good in this patch. My main blocker for that has been access to
RenderView->webview() to case it to WebViewImpl dow the road. Now I can expose a
method in WebViewFrameWidget and move the ownership of TextInputClienObserver to
RenderWidget within this patch.

https://codereview.chromium.org/2278283002/diff/220001/content/renderer/rende...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/2278283002/diff/220001/content/renderer/rende...
content/renderer/render_widget.cc:482: OnTextInputClientMessageReceived(this,
message));
On 2016/09/02 21:57:47, Charlie Reis wrote:
> See my earlier comment.

Acknowledged.

There are 4 IPCs. I think having it temporarily like this might be better. What
will happen here soon is that I will return
text_input_client_observer_->OnMessageReceived() in this block (after ownership
is brought to RenderWidget).

I also need to return after the observer call.

https://codereview.chromium.org/2278283002/diff/220001/content/renderer/rende...
File content/renderer/render_widget.h (right):

https://codereview.chromium.org/2278283002/diff/220001/content/renderer/rende...
content/renderer/render_widget.h:397: // from RenderWidfet. The purpose of
having the reference here was to make IME
On 2016/09/02 21:57:47, Charlie Reis wrote:
> nit: RenderWidget

Done.

https://codereview.chromium.org/2278283002/diff/220001/content/renderer/text_...
File content/renderer/text_input_client_observer.cc (right):

https://codereview.chromium.org/2278283002/diff/220001/content/renderer/text_...
content/renderer/text_input_client_observer.cc:59:
render_frame_->render_view()->webview())
On 2016/09/02 21:57:47, Charlie Reis wrote:
> Are there ever cases that render_view() or webview() can return null?
> 
> We obviously don't want to introduce crashes if that's possible, but we should
> also avoid papering over this if it's never expected to happen.  There's a lot
> of new null checks below due to this, and it's not clear to me if they're
> necessary.

The stuff with checking GetRenderWidget() was absolutely not necessary. We get
here when a RenderWidget receives the IPC so it has to exist here as well.

I assume in that case if the RenderWidget is a RenderView, then we are safe with
WebView reference. If it
is not a RenderView, then it must be a child frame and I guess
RenderView/WebView for the page are created before that?

I tried sending an IPC in the ctor of RWHVMac to see if I might get any crashes
but I could not. I will be investigating this more. However, if lfg@'s patch
lands, we will only have the WebWidget which makes things much cleaner and
easier.

https://codereview.chromium.org/2278283002/diff/220001/content/renderer/text_...
File content/renderer/text_input_client_observer.h (right):

https://codereview.chromium.org/2278283002/diff/220001/content/renderer/text_...
content/renderer/text_input_client_observer.h:31: // RenderViewObserver
overrides:
On 2016/09/02 21:57:47, Charlie Reis wrote:
> Stale comment.  (These now override from somewhere else.)

Done.

https://codereview.chromium.org/2278283002/diff/220001/content/renderer/text_...
content/renderer/text_input_client_observer.h:37: // The render widget
corresponding to the local root.
On 2016/09/02 21:57:47, Charlie Reis wrote:
> This confuses me a bit.  "Local root" refers to a specific frame within a
> widget, so it seems backwards to refer to a widget in terms of a local root.

Acknowledged. I think I meant the RenderFramImpl with the |render_widget_|
equaling |this|.

https://codereview.chromium.org/2278283002/diff/220001/content/renderer/text_...
content/renderer/text_input_client_observer.h:40: // Returns the WebView of the
RenderView.
On 2016/09/02 21:57:48, Charlie Reis wrote:
> Should this still mention RenderView?

No I don't think it should.

Charlie Reis

Thanks! Content/ L.G.T.M. with the changes below, if Ken's happy. I'll wait to give the ...

4 years, 3 months ago (2016-09-09 22:23:32 UTC) #48

Thanks!  Content/ L.G.T.M. with the changes below, if Ken's happy.  I'll wait to
give the actual approval stamp under the assumption that you're waiting for
lfg's CL to land so that this can be cleaned up further.  (Let me know if you
want to land it earlier.)

https://codereview.chromium.org/2278283002/diff/220001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/2278283002/diff/220001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac.mm:63: #include
"content/public/browser/render_widget_host.h"
On 2016/09/08 17:10:40, EhsanK wrote:
> On 2016/09/02 21:57:47, Charlie Reis wrote:
> > Stale?  (Do we need the impl file?)
> 
> impl file is already included on line 46. I think we should keep this as IWYU
> rule. But I am not using this in this patch. But generally this file makes
> references to RenderWidgetHost* so I am not quite sure if we should remove it.
> If so I will remove this along with all the forward decs in this file and the
.h
> file.

Ah, I missed the other uses of RenderWidgetHost in this file.  This can stay.

https://codereview.chromium.org/2278283002/diff/220001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac.mm:100: using
content::RenderWidgetHost;
On 2016/09/08 17:10:40, EhsanK wrote:
> On 2016/09/02 21:57:47, Charlie Reis wrote:
> > Stale?
> 
> Same discussion as the .h file. For the override
> RenderWidgetHostViewMac::GetRenderWidgetHost() we are returning a
> RenderWidgetHost*.

I don't think you need this.  This file is already in content/, so it's a little
weird that we have these "using content::" declarations at all.  I think they're
just for the anonymous namespaces at the top of the file, which don't appear to
include any RenderWidgetHost references.

https://codereview.chromium.org/2278283002/diff/220001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2278283002/diff/220001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:8052: class
SitePerProcessTextInputClientMacTest : public SitePerProcessBrowserTest {
On 2016/09/08 17:10:40, EhsanK wrote:
> On 2016/09/02 21:57:47, Charlie Reis wrote:
> > We should probably put this in a new test file.  This one is already over
8000
> > lines, and it's better not to mix a platform-specific section into it.  :)
> 
> Sure. Is it OK to open up a new file just for this test? I guess we might at
> some point add more tests since TextInputClientMac seems to be needing some
> serious fixes.

Yes, creating a new file for it will be fine.

https://codereview.chromium.org/2278283002/diff/220001/content/public/test/co...
File content/public/test/content_browser_test_utils.h (right):

https://codereview.chromium.org/2278283002/diff/220001/content/public/test/co...
content/public/test/content_browser_test_utils.h:148: void
GetStringAtPointForRenderWidget(
On 2016/09/08 17:10:40, EhsanK wrote:
> On 2016/09/02 21:57:47, Charlie Reis wrote:
> > Please document all new functions in content/public files.
> 
> Acknowledged.

Thanks-- that's really helpful.

https://codereview.chromium.org/2278283002/diff/220001/content/renderer/rende...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/2278283002/diff/220001/content/renderer/rende...
content/renderer/render_frame_impl.h:1281:
std::unique_ptr<TextInputClientObserver> text_input_client_observer_;
On 2016/09/08 17:10:40, EhsanK wrote:
> On 2016/09/02 21:57:47, Charlie Reis wrote:
> > Maybe add a TODO to move this to RenderWidget if that's the intention?
> 
> Done. I added a TODO. That being said, with lfg@'s ongoing patch on returning
a
> WebFrameWidgetBase 
> for RenderWidget::GetWebWidget() I think I will be able to get rid of this for
> good in this patch. My main blocker for that has been access to
> RenderView->webview() to case it to WebViewImpl dow the road. Now I can expose
a
> method in WebViewFrameWidget and move the ownership of TextInputClienObserver
to
> RenderWidget within this patch.

Acknowledged.

kenrb

https://codereview.chromium.org/2278283002/diff/220001/content/browser/renderer_host/render_widget_host_view_mac.mm File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/2278283002/diff/220001/content/browser/renderer_host/render_widget_host_view_mac.mm#newcode2321 content/browser/renderer_host/render_widget_host_view_mac.mm:2321: root_box.bottom_left().y() - view_box.bottom_left().y(); On 2016/09/08 17:10:40, EhsanK wrote: > ...

4 years, 3 months ago (2016-09-12 17:28:13 UTC) #49

https://codereview.chromium.org/2278283002/diff/220001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/2278283002/diff/220001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac.mm:2321:
root_box.bottom_left().y() - view_box.bottom_left().y();
On 2016/09/08 17:10:40, EhsanK wrote:
> On 2016/09/06 18:14:48, kenrb wrote:
> > I don't really understand why this point gets shifted,
> The return coordinate is:
> 1) With respect to topmost local frame (I don't think it is always local
root?)
> 2) inverted y-axis.
> 
> So when it comes back for a child frame we have to account for these things.
> Because of the inverted y-axis
> issue we might not be able to use RWHV::TransformPointToRootCoordSpace.
> Furthermore, the callback here
> is not on UI (I assume IO). So if we want to use that method we have to thread
> hop.

I am surprised to hear about a threading problem, but the point should really be
transformed here. On line 2843 you are transforming the original input point to
the view's coordinate space, and then when Blink returns a point you are doing a
simple translation to return it to the root's space, which isn't necessarily the
inverse (though usually it is). The compositor should internally handle the
inverted y-axis.

If coordinates being received are in the space of the top-most local frame that
is not the local frame root, then that would be a bug. I'll comment on that in
the other file.

> 
> > and in particular why
> > does the code shift a point headed to the top-level frame, even though
> > previously it didn't do that?
> As I understand, we get the RWH at a given point on line 2309 and at the same
> time transform the point in the
> root view to the RWH coordinates (right?). Then we technically transform the
> point before sending the IPC and after we get the response.

The part here that I don't understand is that it looks like you are even
shifting a point that went to a root frame, which wouldn't have had any
transformation done to it before being sent to the renderer process. This isn't
important if you change this to use TransformPointToRootCoordSpace, which is
becomes a nop for the root frame.

https://codereview.chromium.org/2278283002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/mac/WebSubstringUtil.mm (right):

https://codereview.chromium.org/2278283002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/mac/WebSubstringUtil.mm:151: HitTestResult result
= widget->isWebView() ? viewImpl->coreHitTestResultAt(point) :
static_cast<WebFrameWidgetImpl*>(widget)->coreHitTestResultAt(point);
It looks like you dereference widget without first guaranteeing that it isn't
null (you checked that mainFrameImpl() OR widget is null, which isn't
sufficient).

Please check this with lfg@, who has been working on the WebView/WebWidget split
and might have advice on how to structure this.

EhsanK

The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run

4 years, 3 months ago (2016-09-13 17:45:07 UTC) #50

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2278283002/320001

4 years, 3 months ago (2016-09-13 17:45:33 UTC) #51

EhsanK

Thanks for the reviews. PTAL. FYI, I will wait for lfg's patch here: to land ...

4 years, 3 months ago (2016-09-13 18:52:15 UTC) #52

Thanks for the reviews. PTAL.

FYI, I will wait for lfg's patch here:

to land and the rebase again. This will lead to moving TextInputClientObserver
to RenderWidget and also dropping the WebView argument from
WebSubstringUtil::attributedWordAtPoint.

https://codereview.chromium.org/2278283002/diff/220001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/2278283002/diff/220001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac.mm:100: using
content::RenderWidgetHost;
On 2016/09/09 22:23:32, Charlie Reis (slow) wrote:
> On 2016/09/08 17:10:40, EhsanK wrote:
> > On 2016/09/02 21:57:47, Charlie Reis wrote:
> > > Stale?
> > 
> > Same discussion as the .h file. For the override
> > RenderWidgetHostViewMac::GetRenderWidgetHost() we are returning a
> > RenderWidgetHost*.
> 
> I don't think you need this.  This file is already in content/, so it's a
little
> weird that we have these "using content::" declarations at all.  I think
they're
> just for the anonymous namespaces at the top of the file, which don't appear
to
> include any RenderWidgetHost references.

Acknowledged.

https://codereview.chromium.org/2278283002/diff/220001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac.mm:2321:
root_box.bottom_left().y() - view_box.bottom_left().y();
On 2016/09/12 17:28:13, kenrb wrote:
> On 2016/09/08 17:10:40, EhsanK wrote:
> > On 2016/09/06 18:14:48, kenrb wrote:
> > > I don't really understand why this point gets shifted,
> > The return coordinate is:
> > 1) With respect to topmost local frame (I don't think it is always local
> root?)
> > 2) inverted y-axis.
> > 
> > So when it comes back for a child frame we have to account for these things.
> > Because of the inverted y-axis
> > issue we might not be able to use RWHV::TransformPointToRootCoordSpace.
> > Furthermore, the callback here
> > is not on UI (I assume IO). So if we want to use that method we have to
thread
> > hop.
> 
> I am surprised to hear about a threading problem, but the point should really
be
> transformed here. On line 2843 you are transforming the original input point
to
> the view's coordinate space, and then when Blink returns a point you are doing
a
> simple translation to return it to the root's space, which isn't necessarily
the
> inverse (though usually it is). The compositor should internally handle the
> inverted y-axis.
> 
> If coordinates being received are in the space of the top-most local frame
that
> is not the local frame root, then that would be a bug. I'll comment on that in
> the other file.
> 
> > 
> > > and in particular why
> > > does the code shift a point headed to the top-level frame, even though
> > > previously it didn't do that?
> > As I understand, we get the RWH at a given point on line 2309 and at the
same
> > time transform the point in the
> > root view to the RWH coordinates (right?). Then we technically transform the
> > point before sending the IPC and after we get the response.
> 
> The part here that I don't understand is that it looks like you are even
> shifting a point that went to a root frame, which wouldn't have had any
> transformation done to it before being sent to the renderer process. This
isn't
> important if you change this to use TransformPointToRootCoordSpace, which is
> becomes a nop for the root frame.

Following our offline conversation, I am leaving a comment here explaining why
we manually subtract the offset in view bounds rather than using
TransformPointToRootCoordSpace which is due to the unfortunate fact that right
now the IPCs from renderer for this task are handled on IO and the callback is
also on IO.

https://codereview.chromium.org/2278283002/diff/220001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2278283002/diff/220001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:8052: class
SitePerProcessTextInputClientMacTest : public SitePerProcessBrowserTest {
On 2016/09/09 22:23:32, Charlie Reis (slow) wrote:
> On 2016/09/08 17:10:40, EhsanK wrote:
> > On 2016/09/02 21:57:47, Charlie Reis wrote:
> > > We should probably put this in a new test file.  This one is already over
> 8000
> > > lines, and it's better not to mix a platform-specific section into it.  :)
> > 
> > Sure. Is it OK to open up a new file just for this test? I guess we might at
> > some point add more tests since TextInputClientMac seems to be needing some
> > serious fixes.
> 
> Yes, creating a new file for it will be fine.

Added a new file. Thanks!

https://codereview.chromium.org/2278283002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/mac/WebSubstringUtil.mm (right):

https://codereview.chromium.org/2278283002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/mac/WebSubstringUtil.mm:151: HitTestResult result
= widget->isWebView() ? viewImpl->coreHitTestResultAt(point) :
static_cast<WebFrameWidgetImpl*>(widget)->coreHitTestResultAt(point);
On 2016/09/12 17:28:13, kenrb wrote:
> It looks like you dereference widget without first guaranteeing that it isn't
> null (you checked that mainFrameImpl() OR widget is null, which isn't
> sufficient).
> 
> Please check this with lfg@, who has been working on the WebView/WebWidget
split
> and might have advice on how to structure 
I believe neither |view| nor (specifically) |widget| are nullptr when we reach
this point. This call comes from RenderWidget which already owns a WebWidget.
The reason we have a view and widget here is that I cannot cast WebWidget to
WebViewImpl. However, with Lucas's patch coming up I think I will declare the
method coreHitTestResultAt  in WebFrameWidgetBase and implement in both
WebFrameWidgetImpl and WebViewFrameWidget (which would simply call the m_webView
one). With that this ugliness will be removed.

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 3 months ago (2016-09-13 19:56:55 UTC) #53

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/295463)

4 years, 3 months ago (2016-09-13 19:56:57 UTC) #54

EhsanK

Thank you Ken for the reviews. Charlie, Lucas: could you please take another look? (Lucas ...

4 years, 3 months ago (2016-09-14 22:10:32 UTC) #56

lfg

https://codereview.chromium.org/2278283002/diff/380001/content/renderer/text_input_client_observer.cc File content/renderer/text_input_client_observer.cc (right): https://codereview.chromium.org/2278283002/diff/380001/content/renderer/text_input_client_observer.cc#newcode65 content/renderer/text_input_client_observer.cc:65: PepperPluginInstanceImpl* TextInputClientObserver::GetFocusedPepperPlugin() On 2016/09/14 22:10:32, EhsanK wrote: > lfg@: ...

4 years, 3 months ago (2016-09-15 15:51:51 UTC) #57

EhsanK

The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run

4 years, 3 months ago (2016-09-19 18:43:08 UTC) #58

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2278283002/420001

4 years, 3 months ago (2016-09-19 18:43:33 UTC) #59

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 3 months ago (2016-09-19 20:40:02 UTC) #60

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 3 months ago (2016-09-19 20:40:03 UTC) #61

EhsanK

PTAL. https://codereview.chromium.org/2278283002/diff/380001/content/renderer/text_input_client_observer.cc File content/renderer/text_input_client_observer.cc (right): https://codereview.chromium.org/2278283002/diff/380001/content/renderer/text_input_client_observer.cc#newcode65 content/renderer/text_input_client_observer.cc:65: PepperPluginInstanceImpl* TextInputClientObserver::GetFocusedPepperPlugin() On 2016/09/15 15:51:51, lfg wrote: > ...

4 years, 3 months ago (2016-09-19 22:34:05 UTC) #62

EhsanK

ekaramad@chromium.org changed reviewers: + dcheng@chromium.org

4 years, 3 months ago (2016-09-19 22:35:06 UTC) #63

EhsanK

Adding dcheng@ for blink reviews. Daniel, could you please take a look or suggest a ...

4 years, 3 months ago (2016-09-19 22:35:07 UTC) #64

dcheng

blink lgtm https://codereview.chromium.org/2278283002/diff/420001/third_party/WebKit/public/web/mac/WebSubstringUtil.h File third_party/WebKit/public/web/mac/WebSubstringUtil.h (right): https://codereview.chromium.org/2278283002/diff/420001/third_party/WebKit/public/web/mac/WebSubstringUtil.h#newcode45 third_party/WebKit/public/web/mac/WebSubstringUtil.h:45: class WebLocalFrame; Nit: remove this, it appears ...

4 years, 3 months ago (2016-09-21 00:38:34 UTC) #65

Charlie Reis

Sorry for the delay! content/ LGTM. https://codereview.chromium.org/2278283002/diff/420001/content/browser/site_per_process_mac_browsertest.mm File content/browser/site_per_process_mac_browsertest.mm (right): https://codereview.chromium.org/2278283002/diff/420001/content/browser/site_per_process_mac_browsertest.mm#newcode65 content/browser/site_per_process_mac_browsertest.mm:65: class SitePerProcessMacBrowserTest : ...

4 years, 3 months ago (2016-09-23 06:24:10 UTC) #66

EhsanK

Thanks for the reviews! https://codereview.chromium.org/2278283002/diff/420001/content/browser/site_per_process_mac_browsertest.mm File content/browser/site_per_process_mac_browsertest.mm (right): https://codereview.chromium.org/2278283002/diff/420001/content/browser/site_per_process_mac_browsertest.mm#newcode65 content/browser/site_per_process_mac_browsertest.mm:65: class SitePerProcessMacBrowserTest : public SitePerProcessBrowserTest ...

4 years, 2 months ago (2016-09-23 14:36:49 UTC) #67

lfg

Focused plugin lgtm with nit. https://codereview.chromium.org/2278283002/diff/420001/content/renderer/render_view_impl.cc File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/2278283002/diff/420001/content/renderer/render_view_impl.cc#newcode3010 content/renderer/render_view_impl.cc:3010: PepperPluginInstanceImpl* RenderViewImpl::GetFocusedPepperPlugin() { I ...

4 years, 2 months ago (2016-09-23 14:45:36 UTC) #68

EhsanK

Thanks for the reviews! https://codereview.chromium.org/2278283002/diff/420001/content/renderer/render_view_impl.cc File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/2278283002/diff/420001/content/renderer/render_view_impl.cc#newcode3010 content/renderer/render_view_impl.cc:3010: PepperPluginInstanceImpl* RenderViewImpl::GetFocusedPepperPlugin() { On 2016/09/23 ...

4 years, 2 months ago (2016-09-23 15:17:00 UTC) #69

EhsanK

The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run

4 years, 2 months ago (2016-09-23 15:21:25 UTC) #70

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2278283002/440001

4 years, 2 months ago (2016-09-23 15:21:33 UTC) #71

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 2 months ago (2016-09-23 17:08:32 UTC) #72

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 2 months ago (2016-09-23 17:08:34 UTC) #73

EhsanK

The patchset sent to the CQ was uploaded after l-g-t-m from wjmaclean@chromium.org, erikchen@chromium.org, kenrb@chromium.org, dcheng@chromium.org, ...

4 years, 2 months ago (2016-09-23 17:49:32 UTC) #75

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2278283002/440001

4 years, 2 months ago (2016-09-23 17:50:09 UTC) #76

commit-bot: I haz the power

Description was changed from ========== Implement Mac Pop-up Dictionary for OOPIF. Ctrl+Command+D pops up a ...

4 years, 2 months ago (2016-09-23 17:58:06 UTC) #77

commit-bot: I haz the power

Description was changed from ========== Implement Mac Pop-up Dictionary for OOPIF. Ctrl+Command+D pops up a ...

4 years, 2 months ago (2016-09-23 18:00:54 UTC) #79

commit-bot: I haz the power

4 years, 2 months ago (2016-09-23 18:00:56 UTC) #80

Message was sent while issue was closed.

Patchset 21 (id:??) landed as
https://crrev.com/330ba4235e6b32ccad99d48427bc8b7b2df16130
Cr-Commit-Position: refs/heads/master@{#420656}

Issue 2278283002: Implement Mac Pop-up Dictionary for OOPIF. (Closed)

Description

Patch Set 1 #

Patch Set 2 : Fixed a Compile Error #

Patch Set 3 : Fixed Another Compile Error #

Patch Set 4 : Rebased #

Patch Set 5 : Fixed the x-axis error #

Patch Set 6 : Added Tests and Modified DEPS #

Patch Set 7 : Rebased #

Patch Set 8 : Addressing creis@ and erikchen@'s comments #

Patch Set 9 : Some small fixes #

Patch Set 10 : Addressing lfg@'s comment #

Patch Set 11 : Addressing creis@'s comments #

Patch Set 12 : Addressing kenrb@'s comments + fixing a problem for nested in-process <iframe>s #

Patch Set 13 : Rebased #

Patch Set 14 : Added a comment #

Patch Set 15 : Added 'site_per_process_mac_browsertest.mm' #

Patch Set 16 : Fixed a crash #

Patch Set 17 : Rebased #

Patch Set 18 : Moved ownership of TextInputClientObserver to RenderWidget #

Patch Set 19 : Get plugin from focused frame in widget #

Patch Set 20 : Rebased #

Patch Set 21 : Addressing lfg@'s comment #

Messages