Blimp: IME should submit form with text

Blimp: IME should submit form with text

Currently on Blimp IME, hitting Enter only sets the text in the web
page. So user has to hit Enter twice in order to see search results.
This patch solves this by generating a synthetic Enter key press event
and sending it to the renderer.

BUG=611132
Committed: https://crrev.com/46901aa0902ec9336a0eea3b85af83f4b6c7a315
Cr-Commit-Position: refs/heads/master@{#426838}

[shaktisahu, 2016-10-05 20:49:14]

shaktisahu@chromium.org changed reviewers:
+ dtrainor@chromium.org

[Brian Goldman, 2016-10-05 20:51:45]

bgoldman@chromium.org changed reviewers:
+ bgoldman@chromium.org

[Brian Goldman, 2016-10-05 20:51:45]

Can you add test coverage for this? Something in input_browsertest.cc might be
appropriate.

[David Trainor- moved to gerrit, 2016-10-07 06:50:03]

https://codereview.chromium.org/2393043004/diff/1/blimp/engine/feature/engine...
File blimp/engine/feature/engine_render_widget_feature.cc (right):

https://codereview.chromium.org/2393043004/diff/1/blimp/engine/feature/engine...
blimp/engine/feature/engine_render_widget_feature.cc:260: void
EngineRenderWidgetFeature::SetTextFromIME(
Do we want to do this every time?  What if it's a multi-field form?

https://codereview.chromium.org/2393043004/diff/1/blimp/engine/feature/engine...
blimp/engine/feature/engine_render_widget_feature.cc:267: if (!client ||
client->GetTextInputType() == ui::TEXT_INPUT_TYPE_NONE) return;
new line for return.

https://codereview.chromium.org/2393043004/diff/1/blimp/engine/feature/engine...
blimp/engine/feature/engine_render_widget_feature.cc:281: ui::KeyEvent
char_event('\r', ui::VKEY_RETURN, 0);
What's the char event for?

[shaktisahu, 2016-10-07 16:30:08]

https://codereview.chromium.org/2393043004/diff/1/blimp/engine/feature/engine...
File blimp/engine/feature/engine_render_widget_feature.cc (right):

https://codereview.chromium.org/2393043004/diff/1/blimp/engine/feature/engine...
blimp/engine/feature/engine_render_widget_feature.cc:260: void
EngineRenderWidgetFeature::SetTextFromIME(
On 2016/10/07 06:50:03, David Trainor wrote:
> Do we want to do this every time?  What if it's a multi-field form?

We should send a synthetic TAB key instead when there are multiple fields. In
clank, we are already doing that :
https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chro...


But this still doesn't solve much because EditorInfo.IME_ACTION_NEXT action is
not set most of the time even though we have multiple fields. It's only set
today if the field has numbers
https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chro...

https://codereview.chromium.org/2393043004/diff/1/blimp/engine/feature/engine...
blimp/engine/feature/engine_render_widget_feature.cc:281: ui::KeyEvent
char_event('\r', ui::VKEY_RETURN, 0);
On 2016/10/07 06:50:03, David Trainor wrote:
> What's the char event for?

I think for linux, blink expects these three consecutive events in order to
generate a form submission signal. It is same as the sequence of events
generated when we press Enter from the physical keyboard.
Without the char_event, it doesn't work :)

[David Trainor- moved to gerrit, 2016-10-10 21:07:49]

https://codereview.chromium.org/2393043004/diff/1/blimp/engine/feature/engine...
File blimp/engine/feature/engine_render_widget_feature.cc (right):

https://codereview.chromium.org/2393043004/diff/1/blimp/engine/feature/engine...
blimp/engine/feature/engine_render_widget_feature.cc:260: void
EngineRenderWidgetFeature::SetTextFromIME(
On 2016/10/07 16:30:07, shaktisahu wrote:
> On 2016/10/07 06:50:03, David Trainor wrote:
> > Do we want to do this every time?  What if it's a multi-field form?
> 
> We should send a synthetic TAB key instead when there are multiple fields. In
> clank, we are already doing that :
>
https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chro...
> 
> 
> But this still doesn't solve much because EditorInfo.IME_ACTION_NEXT action is
> not set most of the time even though we have multiple fields. It's only set
> today if the field has numbers
>
https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chro...

I don't know if we should auto-select the new field.  I don't think we need that
for now.

https://codereview.chromium.org/2393043004/diff/1/blimp/engine/feature/engine...
blimp/engine/feature/engine_render_widget_feature.cc:281: ui::KeyEvent
char_event('\r', ui::VKEY_RETURN, 0);
On 2016/10/07 16:30:07, shaktisahu wrote:
> On 2016/10/07 06:50:03, David Trainor wrote:
> > What's the char event for?
> 
> I think for linux, blink expects these three consecutive events in order to
> generate a form submission signal. It is same as the sequence of events
> generated when we press Enter from the physical keyboard.
> Without the char_event, it doesn't work :)

Ak ok thanks for explaining.

[shaktisahu, 2016-10-13 21:27:46]

dtrainor@ - PTAL.
I have added a param for submitting the form which will be triggered on hitting
'Enter'.
Also I have bundled the params together into WebInputRequest and
WebInputResponse.

[David Trainor- moved to gerrit, 2016-10-14 16:57:40]

https://codereview.chromium.org/2393043004/diff/20001/blimp/client/core/conte...
File blimp/client/core/contents/android/ime_helper_dialog.cc (right):

https://codereview.chromium.org/2393043004/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/android/ime_helper_dialog.cc:54:
ImeFeature::WebInputResponse response(current_request_);
Can we just put a callback in the request?  Then we can do:

current_request_.text_entered_callback.Run(response)?

https://codereview.chromium.org/2393043004/diff/20001/blimp/client/core/conte...
File
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/ImeHelperDialog.java
(right):

https://codereview.chromium.org/2393043004/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/ImeHelperDialog.java:61:
* @param text The text the user entered.
Add @param submit and describe the behavior.

https://codereview.chromium.org/2393043004/diff/20001/blimp/client/core/conte...
File blimp/client/core/contents/blimp_contents_view_impl_aura.cc (right):

https://codereview.chromium.org/2393043004/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/blimp_contents_view_impl_aura.cc:17: void
OnShowImeRequested(const ImeFeature::WebInputRequest& request) override {
This dropped some behavior (removed callback.Run("")).  We need to trigger the
IME to close right away.

https://codereview.chromium.org/2393043004/diff/20001/blimp/client/core/conte...
File blimp/client/core/contents/ime_feature.cc (right):

https://codereview.chromium.org/2393043004/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/ime_feature.cc:19: render_widget_id(0),
Is 0 the correct invalid id value?  Should we just make the request take these
in the constructor?  If we bake them into the callbcak we can remove them, which
would be nice too :).

https://codereview.chromium.org/2393043004/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/ime_feature.cc:21: text(""),
Don't need.  Class has a default constructor.

https://codereview.chromium.org/2393043004/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/ime_feature.cc:39: DCHECK_LE(0, response.tab_id);
I think generally we put the checked value first and the expected value as the
response?  This feels like it reads a bit cleaner:

DCHECK_GE(response.tab_id, 0);
DCHECK_GT(response.render_wiget_id, 0);

https://codereview.chromium.org/2393043004/diff/20001/blimp/client/core/conte...
File blimp/client/core/contents/ime_feature.h (right):

https://codereview.chromium.org/2393043004/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/ime_feature.h:39: ImeFeature* feature;
I'd add callback members for this instead.  We might not need tab_id or
render_widget_id in that case (they could still be baked into the callback with
closure?).  wdyt?

[shaktisahu, 2016-10-14 18:08:19]

dtrainor@ - PTAL

https://codereview.chromium.org/2393043004/diff/20001/blimp/client/core/conte...
File blimp/client/core/contents/android/ime_helper_dialog.cc (right):

https://codereview.chromium.org/2393043004/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/android/ime_helper_dialog.cc:54:
ImeFeature::WebInputResponse response(current_request_);
On 2016/10/14 16:57:40, David Trainor wrote:
> Can we just put a callback in the request?  Then we can do:
> 
> current_request_.text_entered_callback.Run(response)?

Done.

https://codereview.chromium.org/2393043004/diff/20001/blimp/client/core/conte...
File
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/ImeHelperDialog.java
(right):

https://codereview.chromium.org/2393043004/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/ImeHelperDialog.java:61:
* @param text The text the user entered.
On 2016/10/14 16:57:40, David Trainor wrote:
> Add @param submit and describe the behavior.

Done.

https://codereview.chromium.org/2393043004/diff/20001/blimp/client/core/conte...
File blimp/client/core/contents/ime_feature.cc (right):

https://codereview.chromium.org/2393043004/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/ime_feature.cc:19: render_widget_id(0),
On 2016/10/14 16:57:40, David Trainor wrote:
> Is 0 the correct invalid id value?  Should we just make the request take these
> in the constructor?  If we bake them into the callbcak we can remove them,
which
> would be nice too :).

Done.

https://codereview.chromium.org/2393043004/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/ime_feature.cc:21: text(""),
On 2016/10/14 16:57:40, David Trainor wrote:
> Don't need.  Class has a default constructor.

Done.

https://codereview.chromium.org/2393043004/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/ime_feature.cc:39: DCHECK_LE(0, response.tab_id);
On 2016/10/14 16:57:40, David Trainor wrote:
> I think generally we put the checked value first and the expected value as the
> response?  This feels like it reads a bit cleaner:
> 
> DCHECK_GE(response.tab_id, 0);
> DCHECK_GT(response.render_wiget_id, 0);

I see usages both way. I remember Wez once commented to write this way. Don't
remember exactly though.

https://codereview.chromium.org/2393043004/diff/20001/blimp/client/core/conte...
File blimp/client/core/contents/ime_feature.h (right):

https://codereview.chromium.org/2393043004/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/ime_feature.h:39: ImeFeature* feature;
On 2016/10/14 16:57:40, David Trainor wrote:
> I'd add callback members for this instead.  We might not need tab_id or
> render_widget_id in that case (they could still be baked into the callback
with
> closure?).  wdyt?

Done.

[David Trainor- moved to gerrit, 2016-10-14 18:09:59]

On 2016/10/14 18:08:19, shaktisahu wrote:
> dtrainor@ - PTAL
> 
>
https://codereview.chromium.org/2393043004/diff/20001/blimp/client/core/conte...
> File blimp/client/core/contents/android/ime_helper_dialog.cc (right):
> 
>
https://codereview.chromium.org/2393043004/diff/20001/blimp/client/core/conte...
> blimp/client/core/contents/android/ime_helper_dialog.cc:54:
> ImeFeature::WebInputResponse response(current_request_);
> On 2016/10/14 16:57:40, David Trainor wrote:
> > Can we just put a callback in the request?  Then we can do:
> > 
> > current_request_.text_entered_callback.Run(response)?
> 
> Done.
> 
>
https://codereview.chromium.org/2393043004/diff/20001/blimp/client/core/conte...
> File
>
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/ImeHelperDialog.java
> (right):
> 
>
https://codereview.chromium.org/2393043004/diff/20001/blimp/client/core/conte...
>
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/ImeHelperDialog.java:61:
> * @param text The text the user entered.
> On 2016/10/14 16:57:40, David Trainor wrote:
> > Add @param submit and describe the behavior.
> 
> Done.
> 
>
https://codereview.chromium.org/2393043004/diff/20001/blimp/client/core/conte...
> File blimp/client/core/contents/ime_feature.cc (right):
> 
>
https://codereview.chromium.org/2393043004/diff/20001/blimp/client/core/conte...
> blimp/client/core/contents/ime_feature.cc:19: render_widget_id(0),
> On 2016/10/14 16:57:40, David Trainor wrote:
> > Is 0 the correct invalid id value?  Should we just make the request take
these
> > in the constructor?  If we bake them into the callbcak we can remove them,
> which
> > would be nice too :).
> 
> Done.
> 
>
https://codereview.chromium.org/2393043004/diff/20001/blimp/client/core/conte...
> blimp/client/core/contents/ime_feature.cc:21: text(""),
> On 2016/10/14 16:57:40, David Trainor wrote:
> > Don't need.  Class has a default constructor.
> 
> Done.
> 
>
https://codereview.chromium.org/2393043004/diff/20001/blimp/client/core/conte...
> blimp/client/core/contents/ime_feature.cc:39: DCHECK_LE(0, response.tab_id);
> On 2016/10/14 16:57:40, David Trainor wrote:
> > I think generally we put the checked value first and the expected value as
the
> > response?  This feels like it reads a bit cleaner:
> > 
> > DCHECK_GE(response.tab_id, 0);
> > DCHECK_GT(response.render_wiget_id, 0);
> 
> I see usages both way. I remember Wez once commented to write this way. Don't
> remember exactly though.
> 
>
https://codereview.chromium.org/2393043004/diff/20001/blimp/client/core/conte...
> File blimp/client/core/contents/ime_feature.h (right):
> 
>
https://codereview.chromium.org/2393043004/diff/20001/blimp/client/core/conte...
> blimp/client/core/contents/ime_feature.h:39: ImeFeature* feature;
> On 2016/10/14 16:57:40, David Trainor wrote:
> > I'd add callback members for this instead.  We might not need tab_id or
> > render_widget_id in that case (they could still be baked into the callback
> with
> > closure?).  wdyt?
> 
> Done.

Just as an aside, we should add some tests to this.  Both unit tests and browser
tests.  Should be fairly straightforward to simulate if we create a page that
response to form submission.

[shaktisahu, 2016-10-18 01:33:29]

On 2016/10/14 18:09:59, David Trainor wrote:
> On 2016/10/14 18:08:19, shaktisahu wrote:
> > dtrainor@ - PTAL
> > 
> >
>
https://codereview.chromium.org/2393043004/diff/20001/blimp/client/core/conte...
> > File blimp/client/core/contents/android/ime_helper_dialog.cc (right):
> > 
> >
>
https://codereview.chromium.org/2393043004/diff/20001/blimp/client/core/conte...
> > blimp/client/core/contents/android/ime_helper_dialog.cc:54:
> > ImeFeature::WebInputResponse response(current_request_);
> > On 2016/10/14 16:57:40, David Trainor wrote:
> > > Can we just put a callback in the request?  Then we can do:
> > > 
> > > current_request_.text_entered_callback.Run(response)?
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2393043004/diff/20001/blimp/client/core/conte...
> > File
> >
>
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/ImeHelperDialog.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/2393043004/diff/20001/blimp/client/core/conte...
> >
>
blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/input/ImeHelperDialog.java:61:
> > * @param text The text the user entered.
> > On 2016/10/14 16:57:40, David Trainor wrote:
> > > Add @param submit and describe the behavior.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2393043004/diff/20001/blimp/client/core/conte...
> > File blimp/client/core/contents/ime_feature.cc (right):
> > 
> >
>
https://codereview.chromium.org/2393043004/diff/20001/blimp/client/core/conte...
> > blimp/client/core/contents/ime_feature.cc:19: render_widget_id(0),
> > On 2016/10/14 16:57:40, David Trainor wrote:
> > > Is 0 the correct invalid id value?  Should we just make the request take
> these
> > > in the constructor?  If we bake them into the callbcak we can remove them,
> > which
> > > would be nice too :).
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2393043004/diff/20001/blimp/client/core/conte...
> > blimp/client/core/contents/ime_feature.cc:21: text(""),
> > On 2016/10/14 16:57:40, David Trainor wrote:
> > > Don't need.  Class has a default constructor.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2393043004/diff/20001/blimp/client/core/conte...
> > blimp/client/core/contents/ime_feature.cc:39: DCHECK_LE(0, response.tab_id);
> > On 2016/10/14 16:57:40, David Trainor wrote:
> > > I think generally we put the checked value first and the expected value as
> the
> > > response?  This feels like it reads a bit cleaner:
> > > 
> > > DCHECK_GE(response.tab_id, 0);
> > > DCHECK_GT(response.render_wiget_id, 0);
> > 
> > I see usages both way. I remember Wez once commented to write this way.
Don't
> > remember exactly though.
> > 
> >
>
https://codereview.chromium.org/2393043004/diff/20001/blimp/client/core/conte...
> > File blimp/client/core/contents/ime_feature.h (right):
> > 
> >
>
https://codereview.chromium.org/2393043004/diff/20001/blimp/client/core/conte...
> > blimp/client/core/contents/ime_feature.h:39: ImeFeature* feature;
> > On 2016/10/14 16:57:40, David Trainor wrote:
> > > I'd add callback members for this instead.  We might not need tab_id or
> > > render_widget_id in that case (they could still be baked into the callback
> > with
> > > closure?).  wdyt?
> > 
> > Done.
> 
> Just as an aside, we should add some tests to this.  Both unit tests and
browser
> tests.  Should be fairly straightforward to simulate if we create a page that
> response to form submission.

Added unit tests and browser test. 
PTAL

[Brian Goldman, 2016-10-18 16:46:28]

lgtm. Thanks for setting up DOMMessageQueue; I think it will be a useful example
for other tests.

[David Trainor- moved to gerrit, 2016-10-18 16:53:45]

looking pretty good.  a few more nits :).

https://codereview.chromium.org/2393043004/diff/20001/blimp/client/core/conte...
File blimp/client/core/contents/ime_feature.cc (right):

https://codereview.chromium.org/2393043004/diff/20001/blimp/client/core/conte...
blimp/client/core/contents/ime_feature.cc:39: DCHECK_LE(0, response.tab_id);
On 2016/10/14 18:08:18, shaktisahu wrote:
> On 2016/10/14 16:57:40, David Trainor wrote:
> > I think generally we put the checked value first and the expected value as
the
> > response?  This feels like it reads a bit cleaner:
> > 
> > DCHECK_GE(response.tab_id, 0);
> > DCHECK_GT(response.render_wiget_id, 0);
> 
> I see usages both way. I remember Wez once commented to write this way. Don't
> remember exactly though.

sg

https://codereview.chromium.org/2393043004/diff/60001/blimp/client/core/conte...
File blimp/client/core/contents/ime_feature.cc (right):

https://codereview.chromium.org/2393043004/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/ime_feature.cc:68: &ImeFeature::OnImeTextEntered,
base::Unretained(this),
Hmm base::Unretained() seems incorrect here.  We should probably be using a
WeakPtr.  If this class dies while we have a request out we don't want to call
back into it.

https://codereview.chromium.org/2393043004/diff/60001/blimp/client/core/conte...
File blimp/client/core/contents/ime_feature_unittest.cc (right):

https://codereview.chromium.org/2393043004/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/ime_feature_unittest.cc:25: void
SendMockImeMessage(BlimpMessageProcessor* processor,
Should we also anonymous namespace all of this?

https://codereview.chromium.org/2393043004/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/ime_feature_unittest.cc:29: std::string
existing_text,
const std::string&

https://codereview.chromium.org/2393043004/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/ime_feature_unittest.cc:81:
SendMockImeMessage(&feature_, 0, 1, ui::TEXT_INPUT_TYPE_NONE, std::string(),
""?

https://codereview.chromium.org/2393043004/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/ime_feature_unittest.cc:95: std::string("some text"),
ImeMessage::SHOW_IME);
Don't need to wrap all of these in std::string()

[shaktisahu, 2016-10-18 19:16:00]

dtrainor@ - PTAL

https://codereview.chromium.org/2393043004/diff/60001/blimp/client/core/conte...
File blimp/client/core/contents/ime_feature.cc (right):

https://codereview.chromium.org/2393043004/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/ime_feature.cc:68: &ImeFeature::OnImeTextEntered,
base::Unretained(this),
On 2016/10/18 16:53:45, David Trainor wrote:
> Hmm base::Unretained() seems incorrect here.  We should probably be using a
> WeakPtr.  If this class dies while we have a request out we don't want to call
> back into it.

Done.

https://codereview.chromium.org/2393043004/diff/60001/blimp/client/core/conte...
File blimp/client/core/contents/ime_feature_unittest.cc (right):

https://codereview.chromium.org/2393043004/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/ime_feature_unittest.cc:25: void
SendMockImeMessage(BlimpMessageProcessor* processor,
On 2016/10/18 16:53:45, David Trainor wrote:
> Should we also anonymous namespace all of this?

Done.

https://codereview.chromium.org/2393043004/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/ime_feature_unittest.cc:29: std::string
existing_text,
On 2016/10/18 16:53:45, David Trainor wrote:
> const std::string&

Done.

https://codereview.chromium.org/2393043004/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/ime_feature_unittest.cc:81:
SendMockImeMessage(&feature_, 0, 1, ui::TEXT_INPUT_TYPE_NONE, std::string(),
On 2016/10/18 16:53:45, David Trainor wrote:
> ""?

Done.

https://codereview.chromium.org/2393043004/diff/60001/blimp/client/core/conte...
blimp/client/core/contents/ime_feature_unittest.cc:95: std::string("some text"),
ImeMessage::SHOW_IME);
On 2016/10/18 16:53:45, David Trainor wrote:
> Don't need to wrap all of these in std::string()

Done.

[David Trainor- moved to gerrit, 2016-10-21 16:07:20]

lgtm!

[shaktisahu, 2016-10-21 16:29:23]

The CQ bit was checked by shaktisahu@chromium.org

[shaktisahu, 2016-10-21 16:29:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
bgoldman@chromium.org
Link to the patchset: https://codereview.chromium.org/2393043004/#ps80001
(title: "dtrainor@ comments")

[commit-bot: I haz the power, 2016-10-21 16:29:42]

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

[commit-bot: I haz the power, 2016-10-21 18:04:02]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-21 18:06:58]

Description was changed from

==========
Blimp: IME should submit form with text

Currently on Blimp IME, hitting Enter only sets the text in the web
page. So user has to hit Enter twice in order to see search results.
This patch solves this by generating a synthetic Enter key press event
and sending it to the renderer.

BUG=611132
==========

to

==========
Blimp: IME should submit form with text

Currently on Blimp IME, hitting Enter only sets the text in the web
page. So user has to hit Enter twice in order to see search results.
This patch solves this by generating a synthetic Enter key press event
and sending it to the renderer.

BUG=611132

Committed: https://crrev.com/46901aa0902ec9336a0eea3b85af83f4b6c7a315
Cr-Commit-Position: refs/heads/master@{#426838}
==========

[commit-bot: I haz the power, 2016-10-21 18:06:59]

Patchset 5 (id:??) landed as
https://crrev.com/46901aa0902ec9336a0eea3b85af83f4b6c7a315
Cr-Commit-Position: refs/heads/master@{#426838}