1130284 - Remove redundant updateAppInfo from test files in toolkit/components/search/tests/xpcshell/.

Status: RESOLVED FIXED

(Firefox :: Search, defect, P4)

Description

[Hiroyuki Ikezoe (:hiro)]

Attachment: remove_updateAppInfo.patch

Each test in toolkit/components/search/tests/xpcshell/ invokes updateAppInfo() to register application info but the registration is done in head_search.js. So all of updateAppInfo should be removed.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=02a455d05893

Comment 1

[Marco Bonardo [:mak]]

I think we should also make head_search use updateAppInfo() from AppInfo.jsm rather than implementing its own?

Comment 2

[Hiroyuki Ikezoe (:hiro)]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #1)
> I think we should also make head_search use updateAppInfo() from AppInfo.jsm
> rather than implementing its own?

I was going to fix such change as another bug, but yes you are right. It's much better to change it in this bug.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: remove_updateAppInfo v2

Use updateAppInfo in head_search.js.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b53bafcde5b8

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 8561148 [details] [diff] [review]
remove_updateAppInfo v2

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

::: toolkit/components/search/tests/xpcshell/head_search.js
@@ +52,3 @@
>  var isChild = XULRuntime.processType == XULRuntime.PROCESS_TYPE_CONTENT;
>  
> +updateAppInfo(XULAppInfo);

Is there a valid reason we can't just rely on the default APP_INFO object exposed by AppInfo.jsm if you don't pass into your custom object? (thus just invoking updateAppInfo())

To me looks like it could just work, you would not even need to edit search.json cause the platformBuildID of that object is the same

Comment 5

[Hiroyuki Ikezoe (:hiro)]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #4)
> Comment on attachment 8561148 [details] [diff] [review]
> remove_updateAppInfo v2
> 
> Review of attachment 8561148 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/search/tests/xpcshell/head_search.js
> @@ +52,3 @@
> >  var isChild = XULRuntime.processType == XULRuntime.PROCESS_TYPE_CONTENT;
> >  
> > +updateAppInfo(XULAppInfo);
> 
> Is there a valid reason we can't just rely on the default APP_INFO object
> exposed by AppInfo.jsm if you don't pass into your custom object? (thus just
> invoking updateAppInfo())

Yeah, I was actually going to just invoking updateAppInfo without any argument. But after landing bug 1121340, I think using the default APP_INFO in AppInfo.jsm will break some xpcshell tests. Though it did not break anything for now actually as far as I can push it on try server.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #5)
> as far as I can push it on try server.

I *did* try once.

Comment 7

[Marco Bonardo [:mak]]

well, the only thing that bug changed is

OS: XULRuntime.OS,

this change can very likely be ported to AppInfo.jsm... would be nice if you could try doing that and getting a Try run (running only linux/windows xpcshell should be enough)

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 0: Use XULRuntime.OS instead of "XPCShell" for default app info

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 1: Remove updateAppInfo from search xpcshell tests

Tried on all platforms for safety.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1dc2aa825590

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 8563053 [details] [diff] [review]
Part 0: Use XULRuntime.OS instead of "XPCShell" for default app info

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

::: testing/modules/AppInfo.jsm
@@ +25,5 @@
>    inSafeMode: false,
>    logConsoleErrors: true,
> +  OS: Components.classesByID["{95d89e3e-a169-41a3-8e56-719978e15b12}"]
> +                .getService(Ci.nsIXULRuntime)
> +                .OS,

Rather than getting service everytime OS is requested, I'd put an

XPCOMUtils.defineGetter(this, "XULRuntime",
  () => Components.classesByID["{95d89e3e-a169-41a3-8e56-719978e15b12}"]
	          .getService(Ci.nsIXULRuntime));

Or even better, I'm not sure why this is not using
Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULRuntime)
...
Could you please check if a simple 
XPCOMUtils.defineLazyServiceGetter(this, "XULRuntime",
                                   "@mozilla.org/xre/app-info;1",
                                   "nsIXULRuntime");
would do?

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 8563055 [details] [diff] [review]
Part 1: Remove updateAppInfo from search xpcshell tests

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

yeah, this part looks good now!

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 0: Use XULRuntime.OS instead of "XPCShell" for default app info v2

Use defineLazyServiceGetter.

Seems good.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ba001718f51

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Components.classesByID was used in the search tests because they overrode the default @mozilla.org/xre/app-info;1 implementation, but wanted to reflect a property of the default one.

Comment 14

[Marco Bonardo [:mak]]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #13)
> Components.classesByID was used in the search tests because they overrode
> the default @mozilla.org/xre/app-info;1 implementation, but wanted to
> reflect a property of the default one.

but looks like that doesn't happen anymore, are we fine removing that complication?

Comment 15

[Marco Bonardo [:mak]]

Comment on attachment 8564932 [details] [diff] [review]
Part 0: Use XULRuntime.OS instead of "XPCShell" for default app info v2

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

This looks good to me, by the time we define APP_INFO, XULRuntime is the default, it should work.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

(In reply to Marco Bonardo [::mak] -- currently sick :( (delayed answers) from comment #14)
> but looks like that doesn't happen anymore, are we fine removing that
> complication?

It looks like the result of this patch is that things are re-ordered such that the OS value is obtained before the override occurs, which seems fine.

Comment 17

[Florian Quèze [:florian]]

Is there a reason why this hasn't landed?

Comment 18

[Marco Bonardo [:mak]]

looks like we lost it in the middle of bugzilla, we should check if it bitrotted, and send it to Try.

Comment 19

[Hiroyuki Ikezoe (:hiro)]

Thanks Florian, I did miss this bug.

Unfortunately 'Part 0' patch breaks test_pac_generator.js on B2G.  There needs to be more work....

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb3e08443c81

Comment 20

[Mark Banner (:standard8)]

Hiro, since b2g is no longer being maintained and is being removed (bug 1306391), could you try these again and see if we can land them?

Comment 21

[Hiroyuki Ikezoe (:hiro)]

arai did almost the same thing in bug 1153978.  As far as I can tell what we still need to do is just dropping updateAppInfo() in each test files.  I did push a try with that now:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=42a46226db9be905258d3c4e9d635f9d3dcea4b3

Let's see if it works.

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Attachment: Remove updateAppInfo from search xpcshell tests (updated)

I'd like to ask review again just in case.
Marco?

Comment 23

[Marco Bonardo [:mak]]

I think you meant to link this Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8913a376a915ea9fd2547ba2bbf6bd3eaf0669e

Comment 24

[Marco Bonardo [:mak]]

Comment on attachment 8825613 [details] [diff] [review]
Remove updateAppInfo from search xpcshell tests  (updated)

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

Patch looks ok and Try looks good. Thank you!

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/21846361193f
Remove updateAppInfo from search xpcshell tests. r=mak

Comment 26

[Hiroyuki Ikezoe (:hiro)]

(In reply to Marco Bonardo [::mak] from comment #23)
> I think you meant to link this Try:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=f8913a376a915ea9fd2547ba2bbf6bd3eaf0669e

Right!  It was wrong pasting.  Thanks!

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/21846361193f