Fix AfterTyping experiment for Android.

Fix AfterTyping experiment for Android.

We did not used to set the current url on the Android code path. Instead
of setting the URL explicitly, use the current url specified in the
input instead.

BUG=501529
TBR=pkasting

Committed: https://crrev.com/3ac6843558c83807cc5dd3e62ac5218c350ccd63
Cr-Commit-Position: refs/heads/master@{#335106}

[Maria, 2015-06-17 21:08:43]

mariakhomenko@chromium.org changed reviewers:
+ mpearson@chromium.org

[Mark P, 2015-06-17 21:37:50]

generally lgtm, but one big question:
UpdatePermanentText() gets called a lot more often than SearchProvider::Start().
 We're now only updating the URL at the latter frequency.  Are you sure we
shouldn't be updating it more often?  Redirects?

--mark

[Maria, 2015-06-18 03:42:00]

On 2015/06/17 21:37:50, Mark P wrote:
> generally lgtm, but one big question:
> UpdatePermanentText() gets called a lot more often than
SearchProvider::Start().
>  We're now only updating the URL at the latter frequency.  Are you sure we
> shouldn't be updating it more often?  Redirects?
> 
> --mark

As far as I can tell we are only using the current_url once when we create the
fetcher, which happens in a call chain that happens synchronously from the
Start() call. So even if the url does get updated inside the UpdatePermanentText
call, I don't think the code would pick up the changes. I guess it's possible
that we would get the value set in between when Start() was called and the value
was used, but that would have been at best very racy. I suspect both of these
things are running on UI thread, in which case this wouldn't happen and the
effect is basically that we use whatever value was set when Start() was called.

[Mark P, 2015-06-18 17:38:38]

still lgtm

On Wed, Jun 17, 2015 at 8:42 PM, <mariakhomenko@chromium.org> wrote:

> On 2015/06/17 21:37:50, Mark P wrote:
>
>> generally lgtm, but one big question:
>> UpdatePermanentText() gets called a lot more often than
>>
> SearchProvider::Start().
>
>>   We're now only updating the URL at the latter frequency.  Are you sure
>> we
>> shouldn't be updating it more often?  Redirects?
>>
>
>  --mark
>>
>
> As far as I can tell we are only using the current_url once when we create
> the
> fetcher, which happens in a call chain that happens synchronously from the
> Start() call. So even if the url does get updated inside the
> UpdatePermanentText
> call, I don't think the code would pick up the changes. I guess it's
> possible
> that we would get the value set in between when Start() was called and the
> value
> was used, but that would have been at best very racy. I suspect both of
> these
> things are running on UI thread,

I believe your analysis.  Right you are: both of these things run on the UI
thread.  And you're right that it appears that current_page_url_ is only
used in Start(), which means that getting the value from |input| would be
just as good.  (All updates to current_page_url_ at other times wouldn't do
anything.)

--mark

in which case this wouldn't happen and the
> effect is basically that we use whatever value was set when Start() was
> called.
>
> https://codereview.chromium.org/1189033002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mark P, 2015-06-18 17:38:56]

By the way, thanks for reasoning through that for me. :-)

--mark

On Wed, Jun 17, 2015 at 8:42 PM, <mariakhomenko@chromium.org> wrote:

> On 2015/06/17 21:37:50, Mark P wrote:
>
>> generally lgtm, but one big question:
>> UpdatePermanentText() gets called a lot more often than
>>
> SearchProvider::Start().
>
>>   We're now only updating the URL at the latter frequency.  Are you sure
>> we
>> shouldn't be updating it more often?  Redirects?
>>
>
>  --mark
>>
>
> As far as I can tell we are only using the current_url once when we create
> the
> fetcher, which happens in a call chain that happens synchronously from the
> Start() call. So even if the url does get updated inside the
> UpdatePermanentText
> call, I don't think the code would pick up the changes. I guess it's
> possible
> that we would get the value set in between when Start() was called and the
> value
> was used, but that would have been at best very racy. I suspect both of
> these
> things are running on UI thread, in which case this wouldn't happen and the
> effect is basically that we use whatever value was set when Start() was
> called.
>
> https://codereview.chromium.org/1189033002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mark P, 2015-06-18 17:39:29]

lgtm

[Maria, 2015-06-18 17:47:31]

The CQ bit was checked by mariakhomenko@chromium.org

[commit-bot: I haz the power, 2015-06-18 17:49:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1189033002/1

[commit-bot: I haz the power, 2015-06-18 18:00:00]

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

[commit-bot: I haz the power, 2015-06-18 18:00:01]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Mark P, 2015-06-18 18:03:46]

On 2015/06/18 18:00:01, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)


Oh, apparently I'm not the owner of the UI omnibox code.

I think this is fine to TBR to pkasting, or you can send it to him for official
review.

--mark

[Maria, 2015-06-18 18:18:06]

On 2015/06/18 18:03:46, Mark P wrote:
> On 2015/06/18 18:00:01, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
> 
> 
> Oh, apparently I'm not the owner of the UI omnibox code.
> 
> I think this is fine to TBR to pkasting, or you can send it to him for
official
> review.
> 
> --mark

Ack. Thanks for the review, Mark!

[Maria, 2015-06-18 18:18:14]

The CQ bit was checked by mariakhomenko@chromium.org

[Maria, 2015-06-18 18:18:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
mpearson@chromium.org
Link to the patchset: https://codereview.chromium.org/1189033002/#ps20001
(title: "TBR")

[commit-bot: I haz the power, 2015-06-18 18:21:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1189033002/20001

[commit-bot: I haz the power, 2015-06-18 20:01:25]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-06-18 20:02:21]

Patchset 2 (id:??) landed as
https://crrev.com/3ac6843558c83807cc5dd3e62ac5218c350ccd63
Cr-Commit-Position: refs/heads/master@{#335106}

[Peter Kasting, 2015-06-18 21:41:50]

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

[Peter Kasting, 2015-06-18 21:41:51]

LGTM