952894 - Use existing triggers to clear open search engines state

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: Open search tweaks v0.1

We send a message to clear the tab (and menu) state for open search in Tab.onStateChange:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#3782

But we use the existing location change trigger to clear the same pattern for feeds:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tab.java#639

This patch does the same for open search engines. It also makes some other minor cleanup to make feeds and open search patterns more consistent.

Tab.onStateChange does show up in pageload profiles. Not very high, but every little bit counts. Sending one less message to Java might help a little.

Asking Chenxia for feedback just in case she remembers any reason why we didn't do this to begin with.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: Search and Feed discovery tests v0.1

I ported these tests from two different desktop browser mochitests. I had to do some small tweaks and add a helper to get them to work, but it wasn't too bad.

These only test the Gecko side of search and feed discovery. No Java behavior or UI is tested in this patch. Green on my local test runs. Try run here:
https://tbpl.mozilla.org/?tree=Try&rev=32f340b59bee

Comment 2

[Richard Newman [:rnewman]]

Comment on attachment 8351114 [details] [diff] [review]
Open search tweaks v0.1

Review of attachment 8351114 [details] [diff] [review]:
-----------------------------------------------------------------

It looks like we should also change this (and friends):

Tabs.java:
            } else if (event.equals("Link:OpenSearch")) {
                boolean visible = message.getBoolean("visible");
                tab.setHasOpenSearch(visible);

-- we no longer need the visible flag at all, if we clear the tab state during a location change event, and only send a message when we *do* have a search engine, no?

Comment 3

[Richard Newman [:rnewman]]

Comment on attachment 8351610 [details] [diff] [review]
Search and Feed discovery tests v0.1

Review of attachment 8351610 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/tests/testBrowserDiscovery.js
@@ +15,5 @@
> +// We use a global variable to track the <browser> where the tests are happening
> +let browser;
> +function doc() browser.contentDocument;
> +
> +function setHandlerFunc(resultFunc) {

addLinkAddedHandler(handler)

?

@@ +32,5 @@
> +  let url = "http://mochi.test:8888/tests/robocop/link_discovery.html";
> +  browser = BrowserApp.addTab(url, { selected: true, parentId: BrowserApp.selectedTab.id }).browser;
> +  browser.addEventListener("load", function startTests(event) {
> +    browser.removeEventListener("load", startTests, true);
> +    do_print("XXX starting tests");

This can go, or reword it to not look temporary ("Starting search discovery.")

@@ +37,5 @@
> +    searchDiscovery();
> +  }, true);
> +});
> +
> +var searchDiscoveryTests = [

`let` throughout file.

@@ +60,5 @@
> +  if (browser.engines) {
> +    var hasEngine = (test.count) ? (browser.engines[0].title == title &&
> +                                    browser.engines.length == test.count) :
> +                                   (browser.engines[0].title == title);
> +    ok(hasEngine, test.text);

test.count will be false if zero, for the record. I'd rephrase these four lines as these three:

  let matchCount = !"count" in test || browser.engines.length === test.count;
  let matchTitle = title == browser.engines[0].title;
  ok(matchCount && matchTitle, test.text);

@@ +63,5 @@
> +                                   (browser.engines[0].title == title);
> +    ok(hasEngine, test.text);
> +    browser.engines = null;
> +  } else {
> +    ok(!test.pass, test.text);

Invert and early return:

  if (!browser.engines) {
    ok(!test.pass, test.text);
    return;
  }

  let matchCount = …

@@ +91,5 @@
> +    link.type = type;
> +    link.title = title;
> +    head.appendChild(link);
> +  } else {
> +    feedDiscovery();

This should be the first thing in the function:

  if (!searchDiscoveryTests.length) {
    return feedDiscovery();
  }

But see below.

@@ +126,5 @@
> +    ok(!test.pass, test.text);
> +  }
> +
> +  feedDiscoveryTests.shift();
> +  feedDiscovery(); // Run the next test.

I have to say, I'd rather pre-add all the tests at the top level:

  let test;
  while ((test = searchDiscoveryTests.shift())) {   // Or iteration construct of your choice.
    add_test(do_search_test.bind(this, test));
  }
  while ((test = feedDiscoveryTests.shift())) {
    add_test(do_feed_test.bind(this, test));
  }
  run_next_test();

-- no recursion, no lopsided 'if' inside each Discovery function. Just define:

  function do_search_test(test) {
    let head = doc().getElementById("linkparent");
    setHandlerFunc(runSearchDiscoveryTest, test);
    let link = doc().createElement("link");
    …
  }

No more poking at the global test array. Adjust setHandlerFunc accordingly, of course.

Your stacks will love you :)

@@ +141,5 @@
> +    var rel = test.rel || "alternate";
> +    var href = test.href || "http://so.not.here.mozilla.com/feed.xml";
> +    var type = test.type || "application/atom+xml";
> +    var title = test.title || feedDiscoveryTests.length;
> +    if (test.pass == undefined)

I think you mean

  if (!("pass" in test)) {
    test.pass = true;
  }

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Richard Newman [:rnewman] from comment #2)
> Comment on attachment 8351114 [details] [diff] [review]
> Open search tweaks v0.1
> 
> Review of attachment 8351114 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It looks like we should also change this (and friends):
> 
> Tabs.java:
>             } else if (event.equals("Link:OpenSearch")) {
>                 boolean visible = message.getBoolean("visible");
>                 tab.setHasOpenSearch(visible);
> 
> -- we no longer need the visible flag at all, if we clear the tab state
> during a location change event, and only send a message when we *do* have a
> search engine, no?

Turns out that we do want the visible flag. If a page has two search engines and we add both of those engines, we want to set the Tab.setHasOpenSearch(false) so the menu is hidden. On location changes, we already check for existing search engines, but we still need to do this as the user adds new engines.

We don't do this for feeds. Since we don't control the actual subscription process, like we do with adding search engines, we don't know for certain that the feed was really subscribed. It's not as clear as it is with search engines.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Richard Newman [:rnewman] from comment #3)
> Comment on attachment 8351610 [details] [diff] [review]
> Search and Feed discovery tests v0.1

So much for trying to do a simple copy/paste of a desktop test. Normally I'd punt on test refactor patches, but I thought I'd give it a try.

> ::: mobile/android/base/tests/testBrowserDiscovery.js

> > +function setHandlerFunc(resultFunc) {
> 
> addLinkAddedHandler(handler)

I left the function name but changed the param. Also needed to add a | test | param because of your other suggested changes.

> > +    do_print("XXX starting tests");
> 
> This can go, or reword it to not look temporary ("Starting search
> discovery.")

I removed it. It was debug.

> > +var searchDiscoveryTests = [
> 
> `let` throughout file.

I did the var -> let for all but some global variables.

> > +    var hasEngine = (test.count) ? (browser.engines[0].title == title &&
> > +                                    browser.engines.length == test.count) :
> > +                                   (browser.engines[0].title == title);
> > +    ok(hasEngine, test.text);
> 
> test.count will be false if zero, for the record. I'd rephrase these four
> lines as these three:
> 
>   let matchCount = !"count" in test || browser.engines.length === test.count;
>   let matchTitle = title == browser.engines[0].title;
>   ok(matchCount && matchTitle, test.text);

I made this change, but needed to tweak the matchCount expression:
    let matchCount = (!("count" in test) || browser.engines.length === test.count);

> > +  } else {
> > +    ok(!test.pass, test.text);
> 
> Invert and early return:

Can't return early since we still need to push the tests along with a call to run_next_test()

> I have to say, I'd rather pre-add all the tests at the top level:
> 
>   let test;
>   while ((test = searchDiscoveryTests.shift())) {   // Or iteration
> construct of your choice.
>     add_test(do_search_test.bind(this, test));
>   }
>   while ((test = feedDiscoveryTests.shift())) {
>     add_test(do_feed_test.bind(this, test));
>   }
>   run_next_test();

Done, but I kept the search & feed code groupings. I also needed to set the test.title in these loops. We depend on the title for doing checks.

> Your stacks will love you :)

Sadly, the stacks still sucked. Debugging a test failure isn't easy with all the noise in the output.

> > +    if (test.pass == undefined)
> 
> I think you mean
> 
>   if (!("pass" in test)) {
>     test.pass = true;
>   }

Changed

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: Search and Feed discovery tests v0.2

Green on my local runs. Try run:
https://tbpl.mozilla.org/?tree=Try&rev=1404789486b2

Comment 7

[Richard Newman [:rnewman]]

Comment on attachment 8351669 [details] [diff] [review]
Search and Feed discovery tests v0.2

Review of attachment 8351669 [details] [diff] [review]:
-----------------------------------------------------------------

I have a gentle preference for moving the functions (prep*, execute*) before they're first used. JavaScript will evaluate function definitions elsewhere in the file before doing other evaluations, so this works as written, but still.

::: mobile/android/base/tests/testBrowserDiscovery.js
@@ +1,5 @@
> +// -*- Mode: js2; tab-width: 2; indent-tabs-mode: nil; js2-basic-offset: 2; js2-skip-preprocessor-directives: t; -*-
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +"use strict";

Newline after license.

@@ +13,5 @@
> +}
> +
> +// We use a global variable to track the <browser> where the tests are happening
> +let browser;
> +function doc() browser.contentDocument;

Gentle preference for either (a) inlining this, or (b) just setting a `contentDocument` variable on line 34.

@@ +32,5 @@
> +  let url = "http://mochi.test:8888/tests/robocop/link_discovery.html";
> +  browser = BrowserApp.addTab(url, { selected: true, parentId: BrowserApp.selectedTab.id }).browser;
> +  browser.addEventListener("load", function startTests(event) {
> +    browser.removeEventListener("load", startTests, true);
> +    Services.tm.mainThread.dispatch(run_next_test.bind(this), Ci.nsIThread.DISPATCH_NORMAL);

I don't think r_n_t needs to be bound, here, right? I mean, you're binding it to the `startTests` function, and that ain't gonna help.

@@ +36,5 @@
> +    Services.tm.mainThread.dispatch(run_next_test.bind(this), Ci.nsIThread.DISPATCH_NORMAL);
> +  }, true);
> +});
> +
> +var searchDiscoveryTests = [

Y'know this'll work just fine with `let`, right? :D See line 16!

@@ +62,5 @@
> +
> +function execute_search_test(test) {
> +  if (browser.engines) {
> +    let matchCount = (!("count" in test) || browser.engines.length === test.count);
> +    let matchTitle = (test.title == browser.engines[0].title);

Outer parens not necessary on either line.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Richard Newman [:rnewman] from comment #7)

> I have a gentle preference for moving the functions (prep*, execute*) before
> they're first used. JavaScript will evaluate function definitions elsewhere
> in the file before doing other evaluations, so this works as written, but
> still.

I moved the add_test loops to the bottom of the file

> ::: mobile/android/base/tests/testBrowserDiscovery.js

> > + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +"use strict";
> 
> Newline after license.

Done

> > +function doc() browser.contentDocument;
> 
> Gentle preference for either (a) inlining this, or (b) just setting a
> `contentDocument` variable on line 34.

Inlined

> > +    Services.tm.mainThread.dispatch(run_next_test.bind(this), Ci.nsIThread.DISPATCH_NORMAL);
> 
> I don't think r_n_t needs to be bound, here, right? I mean, you're binding
> it to the `startTests` function, and that ain't gonna help.

Right. This was a "Why the fuck aren't these tests working anymore?" attempted fix. Not needed.

> > +var searchDiscoveryTests = [
> 
> Y'know this'll work just fine with `let`, right? :D See line 16!

Indeed. I am stuck on var of global objects, but these aren't. Switched.

> > +    let matchCount = (!("count" in test) || browser.engines.length === test.count);
> > +    let matchTitle = (test.title == browser.engines[0].title);
> 
> Outer parens not necessary on either line.

True, but it's one of my readability nits.

https://hg.mozilla.org/integration/fx-team/rev/4dcea1e0835f
https://hg.mozilla.org/integration/fx-team/rev/06245a6bf153

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4dcea1e0835f
https://hg.mozilla.org/mozilla-central/rev/06245a6bf153

Comment 10

[Chenxia Liu [:liuche]]

Comment on attachment 8351114 [details] [diff] [review]
Open search tweaks v0.1

Review of attachment 8351114 [details] [diff] [review]:
-----------------------------------------------------------------

I'm late to the party, but there wasn't any particular reason I didn't use the feeds style for open search. Thanks for cleaning it up!

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8351114 [details] [diff] [review]
Open search tweaks v0.1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 852608
User impact if declined: Potential pageload performance issue
Testing completed (on m-c, etc.): It's been on m-c for a while
Risk to taking this patch (and alternatives if risky): Low risk
String or IDL/UUID changes made by this patch: None

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b71255e32a12
https://hg.mozilla.org/releases/mozilla-aurora/rev/de2c488ac317