941565 - OS.Constants.Path.libxul is incorrect on Android

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

OS.Constants.Path.libxul attempts to provide the full path to libxul. We generally don't need this full path, and on Android, getting the full path is a rather complex matter, as it depends on the location of the apk.

We should just provide the library name ("libxul.so", "xul.dll" or "XUL" depending on the platform).

Marking add-on compat in the unlikely case that some add-ons actually use that path information.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Attachment: Fixing OS.Constants.Path.libxul

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Attachment: Fixing OS.Constants.Path.libxul, v2

Comment 3

[Mike Hommey [:glandium]]

Comment on attachment 8336081 [details] [diff] [review]
Fixing OS.Constants.Path.libxul, v2

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

::: dom/system/OSFileConstants.cpp
@@ +814,3 @@
>      xulPath.Append(NS_LITERAL_STRING(DLL_PREFIX));
>      xulPath.Append(NS_LITERAL_STRING("xul"));
>      xulPath.Append(NS_LITERAL_STRING(DLL_SUFFIX));

while you're here, you could make that NS_LITERAL_STRING(DLL_PREFIX "xul" DLL_SUFFIX)

Comment 4

[David Teller [:Yoric] - still alive but not very active]

(In reply to Mike Hommey [:glandium] from comment #3)
> while you're here, you could make that NS_LITERAL_STRING(DLL_PREFIX "xul"
> DLL_SUFFIX)

Actually, that doesn't work with VC++.

Comment 5

[Mike Hommey [:glandium]]

(In reply to David Rajchenbach Teller [:Yoric] <needinfo? me> from comment #4)
> (In reply to Mike Hommey [:glandium] from comment #3)
> > while you're here, you could make that NS_LITERAL_STRING(DLL_PREFIX "xul"
> > DLL_SUFFIX)
> 
> Actually, that doesn't work with VC++.

O_o what does it say?

Comment 6

[David Teller [:Yoric] - still alive but not very active]

I don't remember, but here's an extract from https://developer.mozilla.org/en-US/docs/Mozilla_internal_string_guide: « Because some platforms do runtime conversion, the use of literal string concatenation inside a NS_LITERAL_STRING/NS_NAMED_LITERAL_STRING macro will compile on these platforms, but not on platforms which support compile-time conversion. »

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/b0a4653325cc

Is this something we could have tests for?

Comment 8

[David Teller [:Yoric] - still alive but not very active]

We already have tests for all platforms except Android.
I should be able to cook one for Android. However, the real test will be bug 854169.

Comment 9

[David Teller [:Yoric] - still alive but not very active]

Attachment: Trivial test

Trivial test. Self-reviewing, given the triviality.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage on multiple platforms.
https://hg.mozilla.org/integration/fx-team/rev/299d06e4e8fc

https://tbpl.mozilla.org/php/getParsedLog.php?id=30961089&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=30960686&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=30961182&tree=Fx-Team

I guess you can just roll the test in with the next version of the patch :)

Comment 11

[David Teller [:Yoric] - still alive but not very active]

Attachment: Fixing OS.Constants.Path.libxul, v3

Fixed typo.
Try: https://tbpl.mozilla.org/?tree=Try&rev=1e06dd3f2062

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/88dccf46c960

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/88dccf46c960