Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
322203002:
Add an interface to gather text surrounding the selection. (Closed)
|
Created:
6 years, 6 months ago by Donn Denman Modified:
6 years, 6 months ago Reviewers:
jdduke (slow), David Trainor- moved to gerrit, pedro (no code reviews), Ted C, no sievers, donnd CC:
chromium-reviews, darin-cc_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@surroundings-patch-5 Visibility:
Public. |
DescriptionAdd an interface to gather text surrounding the selection.
This CL is dependent on 292113008 and its dependencies.
BUG=371596, 355154
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278693
Patch Set 1 #
Total comments: 10
Patch Set 2 : Moved surrounding text method definitions to content_view_core.h #Patch Set 3 : Rebase #Patch Set 4 : Reenabled messaging to content_view_core. #Patch Set 5 : Comments. #
Total comments: 16
Patch Set 6 : Pass the delegate into the method that needs it. #Patch Set 7 : Changed to use a callback (but probably not quite right). #Patch Set 8 : Pass the context as a bound parameter in the text surroundings callback. #Patch Set 9 : Add a boolean to track if the text surroundings callback has been set. #Patch Set 10 : Using is_null to check if the callback is null. #
Total comments: 6
Patch Set 11 : Comments, and handle any case where the renderer crashes differently. #
Total comments: 6
Patch Set 12 : Comments, and a DCHECK that a callback was passed in. #Patch Set 13 : Minor format changes done by "git cl format". #
Total comments: 5
Patch Set 14 : Moved the callback to RenderWidgetHostViewAndroid. #
Messages
Total messages: 27 (0 generated)
Jared, Please take a quick look at my approach for exposing surrounding text, built on Mounir's CLs.
On 2014/06/10 21:14:42, Donn Denman wrote: > Jared, Please take a quick look at my approach for exposing surrounding text, > built on Mounir's CLs. Hmm, but how do you plan to expose the delegate implementation to ContentViewCoreImpl? I suppose you could put a setter on the public ContentViewCore native interface for assigning the delegate, but I'm not sure how kosher/standard that is? Dave, do have a standard way of bridging between Android-specific native (C++) code in content/ and external embedders (i.e., downstream native code)?
On 2014/06/10 21:27:27, jdduke wrote: > On 2014/06/10 21:14:42, Donn Denman wrote: > > Jared, Please take a quick look at my approach for exposing surrounding text, > > built on Mounir's CLs. > > Hmm, but how do you plan to expose the delegate implementation to > ContentViewCoreImpl? I suppose you could put a setter on the public > ContentViewCore native interface for assigning the delegate, but I'm not sure > how kosher/standard that is? Dave, do have a standard way of bridging between > Android-specific native (C++) code in content/ and external embedders (i.e., > downstream native code)? David, let me know what you think of this approach -- calling ContentViewCoreImpl::SetTextSurroundingsDelegate() to set the downstream native handler. Let me know if you have a better idea!
https://chromiumcodereview.appspot.com/322203002/diff/1/content/browser/andro... File content/browser/android/content_view_core_impl.h (right): https://chromiumcodereview.appspot.com/322203002/diff/1/content/browser/andro... content/browser/android/content_view_core_impl.h:228: void SetTextSurroundingsDelegate(TextSurroundingsDelegate* delegate) { Ah, so looks like we'll need to have this method exposed on the public ContentViewCore interface. https://chromiumcodereview.appspot.com/322203002/diff/1/content/browser/andro... content/browser/android/content_view_core_impl.h:231: virtual void TextSurroundingSelectionRequest(int max_length); Nit: This should probably read RequestTextSurroundingSelection(), and it looks like the method would also need to live on the ContentViewCore interface. https://chromiumcodereview.appspot.com/322203002/diff/1/content/browser/andro... content/browser/android/content_view_core_impl.h:388: TextSurroundingsDelegate* text_surroundings_delegate_; You'll want to NULL this in the constructor. https://chromiumcodereview.appspot.com/322203002/diff/1/content/browser/andro... File content/browser/android/text_surroundings_delegate.h (right): https://chromiumcodereview.appspot.com/322203002/diff/1/content/browser/andro... content/browser/android/text_surroundings_delegate.h:15: protected: This should probably go in the content/public/android folder where content_view_core.h lives. We may also need a |OnContentViewDestroyed()| method so the delegate doesn't hold any dangling references, OR you could have a |SetContentViewCore()| that gets called when the delegate is assigned to the ContentViewCore (and called with NULL when the ContentViewCore is destroyed).
It seems to me either WebContents or ContentViewCore would work here. However, I don't work enough with downstream code to know best practices here, so I'll defer to dtrainor@.
lgtm with jared/my nits This should be okay but you'll have to keep swapping in/out your delegate every time a new ContentViewCore is selected or gets swapped out (prerendering, etc.). https://chromiumcodereview.appspot.com/322203002/diff/1/content/browser/andro... File content/browser/android/content_view_core_impl.h (right): https://chromiumcodereview.appspot.com/322203002/diff/1/content/browser/andro... content/browser/android/content_view_core_impl.h:229: text_surroundings_delegate_ = delegate; body in cc file except for getters/setters that are hacker style (all lower case, underscore).
Jared, PTAL. Thanks! https://chromiumcodereview.appspot.com/322203002/diff/1/content/browser/andro... File content/browser/android/content_view_core_impl.h (right): https://chromiumcodereview.appspot.com/322203002/diff/1/content/browser/andro... content/browser/android/content_view_core_impl.h:228: void SetTextSurroundingsDelegate(TextSurroundingsDelegate* delegate) { On 2014/06/10 22:08:09, jdduke wrote: > Ah, so looks like we'll need to have this method exposed on the public > ContentViewCore interface. Done. https://chromiumcodereview.appspot.com/322203002/diff/1/content/browser/andro... content/browser/android/content_view_core_impl.h:229: text_surroundings_delegate_ = delegate; On 2014/06/11 17:38:28, David Trainor wrote: > body in cc file except for getters/setters that are hacker style (all lower > case, underscore). Done. https://chromiumcodereview.appspot.com/322203002/diff/1/content/browser/andro... content/browser/android/content_view_core_impl.h:231: virtual void TextSurroundingSelectionRequest(int max_length); On 2014/06/10 22:08:09, jdduke wrote: > Nit: This should probably read RequestTextSurroundingSelection(), and it looks > like the method would also need to live on the ContentViewCore interface. Done. https://chromiumcodereview.appspot.com/322203002/diff/1/content/browser/andro... content/browser/android/content_view_core_impl.h:388: TextSurroundingsDelegate* text_surroundings_delegate_; On 2014/06/10 22:08:09, jdduke wrote: > You'll want to NULL this in the constructor. Done. https://chromiumcodereview.appspot.com/322203002/diff/1/content/browser/andro... File content/browser/android/text_surroundings_delegate.h (right): https://chromiumcodereview.appspot.com/322203002/diff/1/content/browser/andro... content/browser/android/text_surroundings_delegate.h:15: protected: On 2014/06/10 22:08:09, jdduke wrote: > This should probably go in the content/public/android folder where > content_view_core.h lives. We may also need a |OnContentViewDestroyed()| method > so the delegate doesn't hold any dangling references, OR you could have a > |SetContentViewCore()| that gets called when the delegate is assigned to the > ContentViewCore (and called with NULL when the ContentViewCore is destroyed). Done. Looks like the delegate never holds on to the ContentViewCore, so we don't need to notify the delegate when the ContentViewCore goes away. Also, when the ContentViewCore goes away, it will just drop its pointer to the delegate, so we don't even need to NULL or destroy that pointer. Does that sound right to you?
Nice, I think we're almost there, just a few nits and a question. So when you make the query, you just pull a "fresh" version of the ContentViewCore? What if the corresponding ContentViewCore changes, or can that not happen? Then the old contentViewCore will have the dangling delegate reference? Or is the delegate in some way owned by the ContentViewCore, and we can guarantee it will only ever access the one ContentViewCore instance? https://chromiumcodereview.appspot.com/322203002/diff/80001/content/browser/a... File content/browser/android/content_view_core_impl.cc (left): https://chromiumcodereview.appspot.com/322203002/diff/80001/content/browser/a... content/browser/android/content_view_core_impl.cc:249: Hmm, did you remove this line break? If so, maybe undo to reduce git blame noise (can't tell if it was from the rebase). https://chromiumcodereview.appspot.com/322203002/diff/80001/content/browser/a... File content/browser/android/content_view_core_impl.cc (right): https://chromiumcodereview.appspot.com/322203002/diff/80001/content/browser/a... content/browser/android/content_view_core_impl.cc:1551: if (text_surroundings_delegate_ != NULL) { Nit, no need for "!= NULL", you can just do: if (text_surroundings_delegate_) https://chromiumcodereview.appspot.com/322203002/diff/80001/content/browser/a... content/browser/android/content_view_core_impl.cc:1625: RenderFrameHost* focused_frame = web_contents_->GetFocusedFrame(); Should we enforce that |text_surroundings_delegate_| is non-NULL here (i.e., |DCHECK(text_surroundings_delegate_);|)? If it should always be non-NULL for your use-case let's go ahead and add that so that somebody doesn't come along and just start using the API without addressing how the response would be routed. It's OK not to enforce that condition on the response, as the delegate may have gone away in the meantime. https://chromiumcodereview.appspot.com/322203002/diff/80001/content/browser/a... content/browser/android/content_view_core_impl.cc:1625: RenderFrameHost* focused_frame = web_contents_->GetFocusedFrame(); What guarantees do we have that focused_frame will be non-NULL? https://chromiumcodereview.appspot.com/322203002/diff/80001/content/public/br... File content/public/browser/android/content_view_core.h (right): https://chromiumcodereview.appspot.com/322203002/diff/80001/content/public/br... content/public/browser/android/content_view_core.h:11: #include "base/basictypes.h" I don't think you need to add this include. https://chromiumcodereview.appspot.com/322203002/diff/80001/content/public/br... content/public/browser/android/content_view_core.h:75: = 0; "= 0;" should be "OVERRIDE;". https://chromiumcodereview.appspot.com/322203002/diff/80001/content/public/br... File content/public/browser/android/text_surroundings_delegate.h (right): https://chromiumcodereview.appspot.com/322203002/diff/80001/content/public/br... content/public/browser/android/text_surroundings_delegate.h:12: class TextSurroundingsDelegate { I think you'll want to make this: class CONTENT_EXPORT TextSurroundingsDelegate { You'll need to add the "content/common/content_export.h" include. https://chromiumcodereview.appspot.com/322203002/diff/80001/content/public/br... content/public/browser/android/text_surroundings_delegate.h:21: } // namespace content Nit: Newline before the #endif.
I'm still unsure what's best for your concern about connections between the delegate and contentviewcore(s). I think we're OK when a contentviewcore goes away, but your concerns make me worry about the case where the delegate goes away (e.g. the user turns off our feature), which a request is in flight. I would think it would be best to maintain a weak reference to the delegate in the contentviewcore, and check it when the response comes back before delegating. However I have not done this before, and so I tried another option -- just hold on to the delegate for the duration of the async get surroundings call. But with it being so ephemeral, maybe it would be best if it was a callback instead? Can callbacks handle the case where the class to call back goes away? Thanks for all your help with this! https://chromiumcodereview.appspot.com/322203002/diff/80001/content/browser/a... File content/browser/android/content_view_core_impl.cc (left): https://chromiumcodereview.appspot.com/322203002/diff/80001/content/browser/a... content/browser/android/content_view_core_impl.cc:249: On 2014/06/17 22:43:26, jdduke wrote: > Hmm, did you remove this line break? If so, maybe undo to reduce git blame noise > (can't tell if it was from the rebase). Looks like I did -- replaced. https://chromiumcodereview.appspot.com/322203002/diff/80001/content/browser/a... File content/browser/android/content_view_core_impl.cc (right): https://chromiumcodereview.appspot.com/322203002/diff/80001/content/browser/a... content/browser/android/content_view_core_impl.cc:1551: if (text_surroundings_delegate_ != NULL) { On 2014/06/17 22:43:26, jdduke wrote: > Nit, no need for "!= NULL", you can just do: > > if (text_surroundings_delegate_) Done. https://chromiumcodereview.appspot.com/322203002/diff/80001/content/browser/a... content/browser/android/content_view_core_impl.cc:1625: RenderFrameHost* focused_frame = web_contents_->GetFocusedFrame(); On 2014/06/17 22:43:26, jdduke wrote: > What guarantees do we have that focused_frame will be non-NULL? Good idea to check it. I'm cribbing off of Mounir's CL here, so I just figured he'd know, but doesn't hurt to check! I think he figured there needed to be a focused frame to have a selection, but maybe this method could be called without a selection or selected frame. https://chromiumcodereview.appspot.com/322203002/diff/80001/content/browser/a... content/browser/android/content_view_core_impl.cc:1625: RenderFrameHost* focused_frame = web_contents_->GetFocusedFrame(); On 2014/06/17 22:43:26, jdduke wrote: > Should we enforce that |text_surroundings_delegate_| is non-NULL here (i.e., > |DCHECK(text_surroundings_delegate_);|)? If it should always be non-NULL for > your use-case let's go ahead and add that so that somebody doesn't come along > and just start using the API without addressing how the response would be > routed. It's OK not to enforce that condition on the response, as the delegate > may have gone away in the meantime. Done. https://chromiumcodereview.appspot.com/322203002/diff/80001/content/public/br... File content/public/browser/android/content_view_core.h (right): https://chromiumcodereview.appspot.com/322203002/diff/80001/content/public/br... content/public/browser/android/content_view_core.h:11: #include "base/basictypes.h" On 2014/06/17 22:43:26, jdduke wrote: > I don't think you need to add this include. Done. https://chromiumcodereview.appspot.com/322203002/diff/80001/content/public/br... content/public/browser/android/content_view_core.h:75: = 0; On 2014/06/17 22:43:26, jdduke wrote: > "= 0;" should be "OVERRIDE;". Really? The compiler complains about that. Moot point now. https://chromiumcodereview.appspot.com/322203002/diff/80001/content/public/br... File content/public/browser/android/text_surroundings_delegate.h (right): https://chromiumcodereview.appspot.com/322203002/diff/80001/content/public/br... content/public/browser/android/text_surroundings_delegate.h:12: class TextSurroundingsDelegate { On 2014/06/17 22:43:26, jdduke wrote: > I think you'll want to make this: > > class CONTENT_EXPORT TextSurroundingsDelegate { > > You'll need to add the "content/common/content_export.h" include. Done. https://chromiumcodereview.appspot.com/322203002/diff/80001/content/public/br... content/public/browser/android/text_surroundings_delegate.h:21: } // namespace content On 2014/06/17 22:43:26, jdduke wrote: > Nit: Newline before the #endif. Done.
On 2014/06/18 00:09:59, Donn wrote: > I'm still unsure what's best for your concern about connections between the > delegate and contentviewcore(s). I think we're OK when a contentviewcore goes > away, but your concerns make me worry about the case where the delegate goes > away (e.g. the user turns off our feature), which a request is in flight. I > would think it would be best to maintain a weak reference to the delegate in the > contentviewcore, and check it when the response comes back before delegating. > However I have not done this before, and so I tried another option -- just hold > on to the delegate for the duration of the async get surroundings call. But > with it being so ephemeral, maybe it would be best if it was a callback instead? > Can callbacks handle the case where the class to call back goes away? > > Thanks for all your help with this! > I think what we'll want do do here is use a callback function object with each selection request, and it will use a weak reference to to the object being called (the delegate). I'll show you how that's done tomorrow.
> I think what we'll want do do here is use a callback function object with each > selection request, and it will use a weak reference to to the object being > called (the delegate). I'll show you how that's done tomorrow. I took a crack at the callback, but it crashes when it's called, so I know something's not right yet.
On 2014/06/18 01:20:34, Donn Denman wrote: > > I think what we'll want do do here is use a callback function object with each > > selection request, and it will use a weak reference to to the object being > > called (the delegate). I'll show you how that's done tomorrow. > > I took a crack at the callback, but it crashes when it's called, so I know > something's not right yet. Jared, do you want to take another look before I send this to Ted? Thanks!
https://chromiumcodereview.appspot.com/322203002/diff/170001/content/browser/... File content/browser/android/content_view_core_impl.cc (right): https://chromiumcodereview.appspot.com/322203002/diff/170001/content/browser/... content/browser/android/content_view_core_impl.cc:1549: // Re-initialize to the is_null value. In the comments what if we instead say "The callback should be run but once, as it's tied to the request." making it clear why we're resetting the callback. https://chromiumcodereview.appspot.com/322203002/diff/170001/content/browser/... content/browser/android/content_view_core_impl.cc:1624: if (!text_surroundings_callback_.is_null()) I'm not sure if we should be DCHECK'ing, here, but a comment saying "Only one outstanding request is allowed at any given time." might be appropriate. I've been thinking about this some more, it's possible the renderer crashes while the request is made, in which case we never get the response. In that case, the callback will always be non-null, preventing subsequent queries. Not sure what the best solution is there, perhaps we store the callback in RenderWidgetHostViewAndroid, and it gets cleared when we change the ContentViewCore or the renderer crashes. Maybe Ted has some better ideas about managing lifetimes here. https://chromiumcodereview.appspot.com/322203002/diff/170001/content/public/b... File content/public/browser/android/text_surroundings_delegate.h (right): https://chromiumcodereview.appspot.com/322203002/diff/170001/content/public/b... content/public/browser/android/text_surroundings_delegate.h:10: I think you can get rid of this class now.
Jared, PTAL, and then I'll send to Ted. Thanks for all the help on this! https://chromiumcodereview.appspot.com/322203002/diff/170001/content/browser/... File content/browser/android/content_view_core_impl.cc (right): https://chromiumcodereview.appspot.com/322203002/diff/170001/content/browser/... content/browser/android/content_view_core_impl.cc:1549: // Re-initialize to the is_null value. On 2014/06/18 20:32:53, jdduke wrote: > In the comments what if we instead say "The callback should be run but once, as > it's tied to the request." making it clear why we're resetting the callback. Done. https://chromiumcodereview.appspot.com/322203002/diff/170001/content/browser/... content/browser/android/content_view_core_impl.cc:1624: if (!text_surroundings_callback_.is_null()) On 2014/06/18 20:32:53, jdduke wrote: > I'm not sure if we should be DCHECK'ing, here, but a comment saying "Only one > outstanding request is allowed at any given time." might be appropriate. > > I've been thinking about this some more, it's possible the renderer crashes > while the request is made, in which case we never get the response. In that > case, the callback will always be non-null, preventing subsequent queries. Not > sure what the best solution is there, perhaps we store the callback in > RenderWidgetHostViewAndroid, and it gets cleared when we change the > ContentViewCore or the renderer crashes. Maybe Ted has some better ideas about > managing lifetimes here. Glad you caught this possibility! I changed the return when non-null into a DCHECK because I think it would be pretty bad if we blocked subsequent queries. I also put in a comment as you suggested, and I've also added a TODO to capture this issue. I'll ask Ted when passing on for him to review. https://chromiumcodereview.appspot.com/322203002/diff/170001/content/public/b... File content/public/browser/android/text_surroundings_delegate.h (right): https://chromiumcodereview.appspot.com/322203002/diff/170001/content/public/b... content/public/browser/android/text_surroundings_delegate.h:10: On 2014/06/18 20:32:53, jdduke wrote: > I think you can get rid of this class now. Done.
This is probably ready for further OWNER review (and whoever is reviewing your upstream CL should take a look at this). https://chromiumcodereview.appspot.com/322203002/diff/190001/content/browser/... File content/browser/android/content_view_core_impl.cc (right): https://chromiumcodereview.appspot.com/322203002/diff/190001/content/browser/... content/browser/android/content_view_core_impl.cc:1622: RenderFrameHost* focused_frame = web_contents_->GetFocusedFrame(); Let's |DCHECK(!callback.is_null());| here at the beginning as well. https://chromiumcodereview.appspot.com/322203002/diff/190001/content/browser/... content/browser/android/content_view_core_impl.cc:1624: return; Will the delegate be OK if it never gets the callback? In that case, the search would lack context, which shouldn't be a big deal, I just want to make sure the delegate won't be left in a "hanging" state. https://chromiumcodereview.appspot.com/322203002/diff/190001/content/browser/... content/browser/android/content_view_core_impl.cc:1627: // TODO(donnd): consider storing the callback in RenderWidgetHostViewAndroid, Perfect comment, but could you go ahead and file a bug and link to it a the end of the comment (crbug.com/FOO)? We know this could cause problems in practice, so might as well formalize that potential now.
lgtm
Thanks Jared! https://chromiumcodereview.appspot.com/322203002/diff/190001/content/browser/... File content/browser/android/content_view_core_impl.cc (right): https://chromiumcodereview.appspot.com/322203002/diff/190001/content/browser/... content/browser/android/content_view_core_impl.cc:1622: RenderFrameHost* focused_frame = web_contents_->GetFocusedFrame(); On 2014/06/18 23:22:35, jdduke wrote: > Let's |DCHECK(!callback.is_null());| here at the beginning as well. Done. https://chromiumcodereview.appspot.com/322203002/diff/190001/content/browser/... content/browser/android/content_view_core_impl.cc:1624: return; On 2014/06/18 23:22:35, jdduke wrote: > Will the delegate be OK if it never gets the callback? In that case, the search > would lack context, which shouldn't be a big deal, I just want to make sure the > delegate won't be left in a "hanging" state. That should be OK but not ideal -- the context won't get returned and we may not be able to search for this selection, but subsequent requests should be fine. https://chromiumcodereview.appspot.com/322203002/diff/190001/content/browser/... content/browser/android/content_view_core_impl.cc:1627: // TODO(donnd): consider storing the callback in RenderWidgetHostViewAndroid, On 2014/06/18 23:22:34, jdduke wrote: > Perfect comment, but could you go ahead and file a bug and link to it a the end > of the comment (crbug.com/FOO)? We know this could cause problems in practice, > so might as well formalize that potential now. Done. I made this a Restrict-View-Google, but that's probably not correct, right? I wanted to check before changing it.
Ted, would you mind reviewing this? If it's OK, I'll also send the downstream CL that uses this. Thanks!
Ted, wanted to point out an area that could use extra attention (below). https://chromiumcodereview.appspot.com/322203002/diff/230001/content/browser/... File content/browser/android/content_view_core_impl.cc (right): https://chromiumcodereview.appspot.com/322203002/diff/230001/content/browser/... content/browser/android/content_view_core_impl.cc:1629: // TODO(donnd): consider storing the callback in RenderWidgetHostViewAndroid, Ted, we're particularly interested in your opinion of this lifetime issue is, and if you have other suggestions.
https://codereview.chromium.org/322203002/diff/230001/content/browser/android... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/322203002/diff/230001/content/browser/android... content/browser/android/content_view_core_impl.cc:1551: text_surroundings_callback_ = TextSurroundingSelectionCallback(); could this also be .Reset() instead of constructing a new one? https://codereview.chromium.org/322203002/diff/230001/content/browser/android... content/browser/android/content_view_core_impl.cc:1629: // TODO(donnd): consider storing the callback in RenderWidgetHostViewAndroid, On 2014/06/19 14:24:29, Donn Denman wrote: > Ted, we're particularly interested in your opinion of this lifetime issue is, > and if you have other suggestions. Per the offline discussion, I think it is safer to just have the callback be owned by the RenderWidgetHostViewAndroid. Then it will get killed on renderer crashes or when swapping due to a navigation. I think it's a bit odd to go through the frame host and then back out through the render_widget_host_view, but that isn't something for this change.
Ted, PTAL, and thanks for the review and help! https://chromiumcodereview.appspot.com/322203002/diff/230001/content/browser/... File content/browser/android/content_view_core_impl.cc (right): https://chromiumcodereview.appspot.com/322203002/diff/230001/content/browser/... content/browser/android/content_view_core_impl.cc:1551: text_surroundings_callback_ = TextSurroundingSelectionCallback(); On 2014/06/19 21:05:12, Ted C wrote: > could this also be .Reset() instead of constructing a new one? Done. https://chromiumcodereview.appspot.com/322203002/diff/230001/content/browser/... content/browser/android/content_view_core_impl.cc:1629: // TODO(donnd): consider storing the callback in RenderWidgetHostViewAndroid, On 2014/06/19 21:05:12, Ted C wrote: > On 2014/06/19 14:24:29, Donn Denman wrote: > > Ted, we're particularly interested in your opinion of this lifetime issue is, > > and if you have other suggestions. > > Per the offline discussion, I think it is safer to just have the callback be > owned by the RenderWidgetHostViewAndroid. Then it will get killed on renderer > crashes or when swapping due to a navigation. I think it's a bit odd to go > through the frame host and then back out through the render_widget_host_view, > but that isn't something for this change. Done.
lgtm
lgtm
The CQ bit was checked by donnd@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/donnd@chromium.org/322203002/250001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
Message was sent while issue was closed.
Change committed as 278693 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
