Fix copying from interstitial pages on OSX by going through the RenderWidgetHostDelegate.

Fix copying from interstitial pages on OSX by going through the RenderWidgetHostDelegate.

BUG=415784

[lgarron, 2015-05-13 01:08:18]

lgarron@chromium.org changed reviewers:
+ creis@chromium.org, nasko@chromium.org

[lgarron, 2015-05-13 01:08:19]

Take 2, based on Charlie's suggestion in comment #15 of [1].

The first patch only fixes copying, with minimal code changes. I feel like there
are flaws, but I'd appreciate feedback on exactly what.

I'm not sure how many of the other methods to route this way. Any concerns about
doing the same for all similar methods on OSX? {undo, redo, cut, copy,
copyToFindPboard, paste, pasteAndMatchStyle, selectAll} [2][3]


[1] https://codereview.chromium.org/798723004/#msg15
[2]
https://chromium.googlesource.com/chromium/src/+/f76987d989b380c83a8ed3d7c45f...
[3] StartSpeaking and StopSpeaking are actually working well.

[lgarron, 2015-05-13 01:14:06]

Also, WebContentsImpl::Unselect() and WebContentsImpl::Delete() are not called
by render_widget_host_view_mac.mm. Is it fine to ignore those and not implement
them in Interstitial PageImpl?

[nasko, 2015-05-13 17:24:08]

Thanks for implementing it with this pattern. Overall looks fine with some
changes needed.
I didn't see any tests for this, are you planning to add them in a later
revision of this CL?

https://codereview.chromium.org/1133003003/diff/20001/content/browser/frame_h...
File content/browser/frame_host/interstitial_page_impl.cc (right):

https://codereview.chromium.org/1133003003/diff/20001/content/browser/frame_h...
content/browser/frame_host/interstitial_page_impl.cc:209:
focused_frame->Send(new InputMsg_Redo(focused_frame->GetRoutingID()));
Incorrect indentation. In general, you can use "git cl format" to take care of
style formatting, so you don't have to think about it.

https://codereview.chromium.org/1133003003/diff/20001/content/browser/frame_h...
File content/browser/frame_host/interstitial_page_impl.h (right):

https://codereview.chromium.org/1133003003/diff/20001/content/browser/frame_h...
content/browser/frame_host/interstitial_page_impl.h:60: void Undo() override;
Those methods aren't part of the InterstitialPage interface, so they shouldn't
be under its heading. Please add // RenderWidgetHostDelegate implementation:
heading before those methods.

https://codereview.chromium.org/1133003003/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_delegate.h (right):

https://codereview.chromium.org/1133003003/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_delegate.h:78: virtual void
Undo();
Each method needs a comment, just like the rest of the methods above it.

https://codereview.chromium.org/1133003003/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/1133003003/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.h:427:
RenderWidgetHostDelegate* GetDelegate();
Simple accessors, such as this one, use hacker_case as opposed to CamelCase.

https://codereview.chromium.org/1133003003/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/1133003003/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:3262:
render_widget_host_delegate->Undo();
I think it is possible for the delegate of RenderWidgetHost to be nullptr, which
means this will crash in those cases.

[lgarron, 2015-05-13 21:58:41]

Yep, this CL doesn't have tests yet. I barely had things working when I made it.
:-P

I'm thinking of two tests:

- Test that the interstitial page has a focused frame, similar to
https://codereview.chromium.org/798723004
- Create a browser test that triggers a copy action, and mock out the copy even
to make sure that it is triggered.

Do those sound sensible?
(Is there a point in the first if we test the second one?)

https://codereview.chromium.org/1133003003/diff/20001/content/browser/frame_h...
File content/browser/frame_host/interstitial_page_impl.cc (right):

https://codereview.chromium.org/1133003003/diff/20001/content/browser/frame_h...
content/browser/frame_host/interstitial_page_impl.cc:209:
focused_frame->Send(new InputMsg_Redo(focused_frame->GetRoutingID()));
On 2015/05/13 at 17:24:08, nasko wrote:
> Incorrect indentation. In general, you can use "git cl format" to take care of
style formatting, so you don't have to think about it.

Done.

(Not sure how that happened. In any case, I ran `git cl format` again, and this
was the only thing that changed.)

https://codereview.chromium.org/1133003003/diff/20001/content/browser/frame_h...
File content/browser/frame_host/interstitial_page_impl.h (right):

https://codereview.chromium.org/1133003003/diff/20001/content/browser/frame_h...
content/browser/frame_host/interstitial_page_impl.h:60: void Undo() override;
On 2015/05/13 at 17:24:08, nasko wrote:
> Those methods aren't part of the InterstitialPage interface, so they shouldn't
be under its heading. Please add // RenderWidgetHostDelegate implementation:
heading before those methods.

Done.

https://codereview.chromium.org/1133003003/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_delegate.h (right):

https://codereview.chromium.org/1133003003/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_delegate.h:78: virtual void
Undo();
On 2015/05/13 at 17:24:08, nasko wrote:
> Each method needs a comment, just like the rest of the methods above it.

I've added "// Send the OS Undo action to the focused frame."
Is that the right amount of detail?

https://codereview.chromium.org/1133003003/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/1133003003/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.h:427:
RenderWidgetHostDelegate* GetDelegate();
On 2015/05/13 at 17:24:08, nasko wrote:
> Simple accessors, such as this one, use hacker_case as opposed to CamelCase.

get_delegate() or just delegate()?

https://codereview.chromium.org/1133003003/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/1133003003/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:3262:
render_widget_host_delegate->Undo();
On 2015/05/13 at 17:24:08, nasko wrote:
> I think it is possible for the delegate of RenderWidgetHost to be nullptr,
which means this will crash in those cases.

I've added a simple `if (!render_widget_host_delegate)` check.

(If that's not the idiomatic way to do this, let me know.)

[lgarron, 2015-05-14 01:31:21]

I'm a bit lost on how exactly to test this.

There's an ssl_browser_tests.cc class that looks appropriate, but I'm wondering
about how to trigger the copy action.
After looking at it with Mustafa, I was considering doing something like
SendKeyEvent [1] from another test, but I've been staring at that code and not
coming up with a great way to reuse it.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...

Another option might be to test the copy: method in
render_widget_host_view_mac.mm, but that file only has unit tests, and I don't
know how I would start testing interstitials there.

Any sage advice?

[Charlie Reis, 2015-05-14 20:36:27]

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

[Charlie Reis, 2015-05-14 20:36:28]

[+kenrb]

Thanks for switching to this approach!

Ken, do you have any advice for testing input events like copy/paste?

Otherwise mostly looks good with nits.

https://codereview.chromium.org/1133003003/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_delegate.h (right):

https://codereview.chromium.org/1133003003/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_delegate.h:78: virtual void
Undo();
On 2015/05/13 21:58:40, lgarron wrote:
> On 2015/05/13 at 17:24:08, nasko wrote:
> > Each method needs a comment, just like the rest of the methods above it.
> 
> I've added "// Send the OS Undo action to the focused frame."
> Is that the right amount of detail?

Sounds good.

https://codereview.chromium.org/1133003003/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/1133003003/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.h:427:
RenderWidgetHostDelegate* GetDelegate();
On 2015/05/13 21:58:40, lgarron wrote:
> On 2015/05/13 at 17:24:08, nasko wrote:
> > Simple accessors, such as this one, use hacker_case as opposed to CamelCase.
> 
> get_delegate() or just delegate()?

delegate().

They should also be inlined in the .h file, similar to surface_id() on line 179.

https://codereview.chromium.org/1133003003/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/1133003003/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:3262: if
(!render_widget_host_delegate) {
nit: Seems like this would be easier to read, like before:

if (render_widget_host_delegate)
  render_widget_host_delegate->Undo();

[nasko, 2015-05-14 20:45:23]

I need to think/dig for ideas for testing. Just a few comments

https://codereview.chromium.org/1133003003/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_delegate.h (right):

https://codereview.chromium.org/1133003003/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_delegate.h:78: virtual void
Undo();
On 2015/05/13 21:58:40, lgarron wrote:
> On 2015/05/13 at 17:24:08, nasko wrote:
> > Each method needs a comment, just like the rest of the methods above it.
> 
> I've added "// Send the OS Undo action to the focused frame."
> Is that the right amount of detail?

That looks fine.

https://codereview.chromium.org/1133003003/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/1133003003/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.h:427:
RenderWidgetHostDelegate* GetDelegate();
On 2015/05/13 21:58:40, lgarron wrote:
> On 2015/05/13 at 17:24:08, nasko wrote:
> > Simple accessors, such as this one, use hacker_case as opposed to CamelCase.
> 
> get_delegate() or just delegate()?

Just delegate().

https://codereview.chromium.org/1133003003/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/1133003003/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:3262:
render_widget_host_delegate->Undo();
On 2015/05/13 21:58:41, lgarron wrote:
> On 2015/05/13 at 17:24:08, nasko wrote:
> > I think it is possible for the delegate of RenderWidgetHost to be nullptr,
> which means this will crash in those cases.
> 
> I've added a simple `if (!render_widget_host_delegate)` check.
> 
> (If that's not the idiomatic way to do this, let me know.)

Why not:

if (render_widget_host_delegate)
  render_widget_host_delegate->call()

[kenrb, 2015-05-15 20:48:06]

On 2015/05/14 20:36:28, Charlie Reis wrote:
> [+kenrb]
> 
> Thanks for switching to this approach!
> 
> Ken, do you have any advice for testing input events like copy/paste?
> 

I'm not sure, but my best guess would be to add a test to
render_widget_host_view_mac_unittest.mm that triggers the copy and paste events,
although there might be some extra setup work to have an interstitial page.

[lgarron, 2015-07-10 01:05:50]

On 2015/05/15 at 20:48:06, kenrb wrote:
> On 2015/05/14 20:36:28, Charlie Reis wrote:
> > [+kenrb]
> > 
> > Thanks for switching to this approach!
> > 
> > Ken, do you have any advice for testing input events like copy/paste?
> > 
> 
> I'm not sure, but my best guess would be to add a test to
render_widget_host_view_mac_unittest.mm that triggers the copy and paste events,
although there might be some extra setup work to have an interstitial page.

Closing in favor of https://codereview.chromium.org/1162373002 and
https://codereview.chromium.org/1212373009