Shing Lyu [:shinglyu] Reporter
	Description • 9 years ago

*** Description
Search by clicking the search suggestions are not counted in the app usage ping.

*** Steps to Reproduce
Pre-requisite: the search suggestion is enabled.

1. Click the rocket bar
2. Type "Mozilla"
3. Click the first search suggestions 
4. Wait for the App Usage ping to reach the metrics server

*** Expected Results
Default search provider has a search count of 1

*** Actual Results
Search count = 0

*** Other Notes
Only the searchs performed by clicking the "magnifier glass" button on the keyboard are counted. Users are equally probable to use both methods, so the search count will be underestimated

*** Reproduction Frequency


*** Build

	Shing Lyu [:shinglyu] Reporter
	Comment 1 • 9 years ago

*** Reproduction Frequency
3/3

*** Build
  Gaia-Rev        3f032238a52f08e4c2f68a47ad065a96eb22d470
  Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/9ae4bcc9b5f2
  Build-ID        20150309162503
  Version         37.0
  Device-Name     flame
  FW-Release      4.4.2
  FW-Incremental  eng.cltbld.20150309.201258
  FW-Date         Mon Mar  9 20:13:09 EDT 2015
  Bootloader      L1TC000118D0

	bhavana bajaj [:bajaj]
	Comment 2 • 9 years ago

Looks like its a broken functionality from a new metrics related feature, so blocking on this

	howie [:howie]
	Comment 4 • 9 years ago

Hi Tamara, so you are working on this? thanks.

	Tamara Hills [:thills] Assignee
	Comment 5 • 9 years ago

Comment on attachment 8576401 [details] [review]
[gaia] tamarahills:bugfix/1141891-add-suggestion-search > mozilla-b2g:master

Hi Marshall, sorry for the confusion.  I originally inadvertently posted this patch on the wrong bug.  I was wondering why the autolander didn't show up :)

	Tamara Hills [:thills] Assignee
	Comment 6 • 9 years ago

Hi Howie,

Yes, sorry I forgot to set to assigned.  I have posted a PR as well.

Thanks,
-tamara

	Marshall Culpepper [:marshall_law]
	Comment 7 • 9 years ago

Comment on attachment 8576401 [details] [review]
[gaia] tamarahills:bugfix/1141891-add-suggestion-search > mozilla-b2g:master

Looks great! We should probably have someone that works on search take a look too..

	Tamara Hills [:thills] Assignee
	Comment 8 • 9 years ago

Comment on attachment 8576401 [details] [review]
[gaia] tamarahills:bugfix/1141891-add-suggestion-search > mozilla-b2g:master

Hi Dale,

Would you be able to have a look?

Thanks,

-tamara

	Dale Harvey (:daleharvey)
	Comment 9 • 9 years ago

Comment on attachment 8576401 [details] [review]
[gaia] tamarahills:bugfix/1141891-add-suggestion-search > mozilla-b2g:master

This looks great, cheers

	Tamara Hills [:thills] Assignee
	Comment 10 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=gaia-try&revision=903e6f09d2db
https://github.com/mozilla-b2g/gaia/commit/db48e2353effeb3e3f74dda7b9103ee7ce8bbfde

	Tamara Hills [:thills] Assignee
	Comment 11 • 9 years ago

Comment on attachment 8576401 [details] [review]
[gaia] tamarahills:bugfix/1141891-add-suggestion-search > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Search count metrics feature
[User impact] if declined: Suggested searches will not be counted and we will significantly undercount the metrics if we don't have this
[Testing completed]: yes
[Risk to taking this patch] (and alternatives if risky): low-medium.  Should be easy to backout as it's small.
[String changes made]:no

	Ryan VanderMeulen [:RyanVM]
	Comment 12 • 9 years ago

v2.2: https://github.com/mozilla-b2g/gaia/commit/a668608eae3183ab232c54d1a1d342555b1d9c56

	Ryan VanderMeulen [:RyanVM]
	Comment 13 • 9 years ago

Reverted for Gaia unit test failures.
v2.2: https://github.com/mozilla-b2g/gaia/commit/fd7fa0c8f7ca575aafc3568262247e529323f31f

https://treeherder.mozilla.org/logviewer.html#?job_id=73272&repo=mozilla-b2g37_v2_2

	Tamara Hills [:thills] Assignee
	Comment 14 • 9 years ago

Hi Dale,

I'm having some trouble with my suggestions_test.js when it gets uplifted to 2.2 and realized that this line (SearchProvider.ready()) is removed in master, but still exists in 2.2

master:
https://github.com/mozilla-b2g/gaia/commit/323d773af7c579cc7f33fbe571cc5c8631156b15#diff-a72719dbedcb33e670a082ee4341d876L36

2.2:
https://github.com/mozilla-b2g/gaia/blob/v2.2/apps/search/js/providers/suggestions.js#L36

It looks like bug 1131966 removes the line, but that change doesn't seem to be in 2.2.  Just wanted to know if this will be uplifted before I redo the unit test to work differently with 2.2.

Thanks!

-tamara

	Ryan VanderMeulen [:RyanVM]
	Comment 15 • 9 years ago

v2.2: https://github.com/mozilla-b2g/gaia/commit/3c5565bcdf0045261e0fd76e700eb32c4bbf3dc0

	Ryan VanderMeulen [:RyanVM]
	Comment 16 • 9 years ago

Unfortunately, this is still hitting test failures on v2.2 even with bug 1136664 uplifted first :(. Backed out again.
v2.2: https://github.com/mozilla-b2g/gaia/commit/f52a2ac8f42489587e95a9254045683e0454b2e6

https://treeherder.mozilla.org/logviewer.html#?job_id=76315&repo=mozilla-b2g37_v2_2

	Ryan VanderMeulen [:RyanVM]
	Comment 17 • 9 years ago

Ugh, I obviously can't read. I have no idea why I thought bug 1136664 was going to help here.

	Shing Lyu [:shinglyu] Reporter
	Comment 18 • 9 years ago

Hi Tamara, 

Can we uplift this to 2.2?

	Dale Harvey (:daleharvey)
	Comment 19 • 9 years ago

So somehow a line that was removed in master did not get removed in 2.2

https://github.com/mozilla-b2g/gaia/commit/323d773af7c579cc7f33fbe571cc5c8631156b15 is the master commit, https://github.com/mozilla-b2g/gaia/commit/df92a3c0d8ed757c04b81085c260da9234c59981 is the 2.2 one

The |SearchProvider.ready()| was removed in master but not in the 2.2 commit, a little confused as to how that can happen since it wasnt a branch patch but I am not a git expert, Ryan do you have any ideas?

Tamara is gonna do a branch patch to remove that and hopefully the existing test should work as is.

	Ryan VanderMeulen [:RyanVM]
	Comment 20 • 9 years ago

No clue, sorry.

	Tamara Hills [:thills] Assignee
	Comment 22 • 9 years ago

Comment on attachment 8582392 [details] [review]
[gaia] tamarahills:bugfix/1141891-add-suggestion-search-2.2-branch-patch > mozilla-b2g:v2.2

Hi Marshall,

I talked with Dale and we decided best approach is to make a branch patch.  Essentially, there was a merge conflict from bug 1131966 where a merge didn't get correctly resolved on the uplift.  So, we decided I should remove this line in the branch patch.  

BTW, I'm not sure what the process is for branch patch review, so just adding a review flag on this.  It's the same patch as the 3.0, except with this line removed: https://github.com/mozilla-b2g/gaia/pull/29097/files#diff-a72719dbedcb33e670a082ee4341d876L36

For reference here is the merge commit difference on bug 1131966
master commit:  https://github.com/mozilla-b2g/gaia/commit/323d773af7c579cc7f33fbe571cc5c8631156b15

2.2 commit:  https://github.com/mozilla-b2g/gaia/commit/df92a3c0d8ed757c04b81085c260da9234c59981

	Marshall Culpepper [:marshall_law]
	Comment 23 • 9 years ago

Comment on attachment 8582392 [details] [review]
[gaia] tamarahills:bugfix/1141891-add-suggestion-search-2.2-branch-patch > mozilla-b2g:v2.2

Looks good, just had one quick question about a removed line of code

	Ryan VanderMeulen [:RyanVM]
	Comment 24 • 9 years ago

v2.2: https://github.com/mozilla-b2g/gaia/commit/a7ff88fb4dec59533dfe7194ff9bf760d4de3eae