949944 - [B2G][Helix][Browser][Wallpaper] use new File([blob], filename) to return a blob with filename when picking

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Wallpaper, defect, P2)

Description

[John Hu [:johnhu][:johu][:醬糊小弟]]

+++ This bug was initially created as a clone of Bug #920905 +++

When handling pick activity, we need to use new File([blob], filename) to return a blob with readable file name.

Comment 1

[John Hu [:johnhu][:johu][:醬糊小弟]]

nominate it as 1.3? since its original bug is 1.3?.

Comment 2

[John Hu [:johnhu][:johu][:醬糊小弟]]

Hi Baku,

I found a strange error when picking from wallpaper with/without using new File([blob], filename). I had dumped the returned filename and error message:

E/GeckoConsole( 2602): Content JS LOG at app://wallpaper.gaiamobile.org/js/pick.js:68 in wallpaper_pickWallpaper/img.onload/<: src resources/320x480/FXOS_Illus_Blocks.png

E/GeckoConsole( 2552): [JavaScript Error: "NS_ERROR_FILE_UNRECOGNIZED_PATH: Component returned failure code: 0x80520001 (NS_ERROR_FILE_UNRECOGNIZED_PATH) [nsILocalFile.initWithPath]" {file: "jar:file:///system/b2g/omni.ja!/components/FilePicker.js" line: 194}]
E/GeckoConsole( 2082): Content JS WARN at app://system.gaiamobile.org/js/activity_window.js:202 in onClose: unknown window type of activity caller.

BTW, I found wallpaper returns its file name with result.name. Does that mean I don't need to return it with new File() according to [1]?

[1] http://mxr.mozilla.org/mozilla-central/source/b2g/components/FilePicker.js#185

Comment 3

[John Hu [:johnhu][:johu][:醬糊小弟]]

BTW, I found another bug at [1]. We should use let file = new FileUtils.File(name);

[1] http://mxr.mozilla.org/mozilla-central/source/b2g/components/FilePicker.js#194

Comment 4

[John Hu [:johnhu][:johu][:醬糊小弟]]

Attachment: possible patch

Baku,

I did some tests and found the "new FileUtils.File()" needs absolute file path. So, I use pure JavaScript to take the file name out. Any suggestions?

Comment 5

[Andrea Marchesini [:baku]]

This is an approach that works for B2G. So I guess it's fine. I want to have fabrice to review this patch.

Comment 6

[John Hu [:johnhu][:johu][:醬糊小弟]]

Attachment: bug-949944-fix.patch

Fabrice,

Please review this patch, according to the suggestion of baku at comment 5. The main thought of this patch is to use javascript function to take the filename out instead of FileUtils.File().

Comment 7

[[:fabrice] Fabrice Desré]

Comment on attachment 8348493 [details] [diff] [review]
bug-949944-fix.patch

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

::: b2g/components/FilePicker.js
@@ +190,5 @@
>      }
>  
>      // Let's try to remove the full path and take just the filename.
>      if (name) {
> +      name = name.substr(name.lastIndexOf('/') + 1);

this will not work on windows, which we support in desktop builds.

Comment 8

[John Hu [:johnhu][:johu][:醬糊小弟]]

Attachment: bug-949944-fix-v2.patch

Thank you for telling me this. I thought b2g folder is only for b2g project in the past. I think I am wrong about it. b2g folder may also be used in b2g-desktop and others.

This patch uses OS.Path.split to get the filename. Please review it, thanks.

Comment 9

[[:fabrice] Fabrice Desré]

Comment on attachment 8349848 [details] [diff] [review]
bug-949944-fix-v2.patch

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

Can we get tests for that?

Comment 10

[Andrea Marchesini [:baku]]

Comment on attachment 8349848 [details] [diff] [review]
bug-949944-fix-v2.patch

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

::: b2g/components/FilePicker.js
@@ +32,4 @@
>                         'audio/webm'];
>  
>  Cu.import('resource://gre/modules/XPCOMUtils.jsm');
> +Cu.import('resource://gre/modules/FileUtils.jsm');

Do we need this line?

Comment 11

[John Hu [:johnhu][:johu][:醬糊小弟]]

(In reply to Andrea Marchesini (:baku) from comment #10)
> > +Cu.import('resource://gre/modules/FileUtils.jsm');
> 
> Do we need this line?

No, we don't need this line. I will remove it.

Comment 12

[John Hu [:johnhu][:johu][:醬糊小弟]]

Ok, I will try to write tests for this patch....

Comment 13

[John Hu [:johnhu][:johu][:醬糊小弟]]

Attachment: bug-949944-v3.patch

Remove the useless import, as per comment 10, and move the r+ here.

Comment 14

[John Hu [:johnhu][:johu][:醬糊小弟]]

Attachment: bug-949944-v4.patch

Fabrice,

I had merged the tests into the patch. Please review it again.

And Shih-Chiang Chien told me that this test needs to run at OOP environment to have correct result. So, I only put tests into b2g and b2g-debug.

Thanks.

Comment 15

[John Hu [:johnhu][:johu][:醬糊小弟]]

Attachment: bug-949944-v4.1.patch

Found additional spaces at v4.

Comment 16

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 8351354 [details] [diff] [review]
bug-949944-v4.1.patch


Fabrice,

I had merged the tests into the patch. Please review it again.

And Shih-Chiang Chien told me that this test needs to run at OOP environment to have correct result. So, I only put tests into b2g and b2g-debug.

Thanks

Comment 17

[John Hu [:johnhu][:johu][:醬糊小弟]]

Attachment: bug-949944-v5.patch

Comment 18

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 8351355 [details] [diff] [review]
bug-949944-v5.patch


Fabrice,

I had merged the tests into the patch. Please review it again.

And Shih-Chiang Chien told me that this test needs to run at OOP environment to have correct result. So, I only put tests into b2g and b2g-debug.

Thanks

Comment 19

[[:fabrice] Fabrice Desré]

Comment on attachment 8351355 [details] [diff] [review]
bug-949944-v5.patch

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

That looks good, thanks! Can you check the windows path before landing?

::: b2g/components/test/mochitest/filepicker_path_handler_chrome.js
@@ +8,5 @@
> +const Ci = Components.interfaces;
> +
> +// use ppmm to handle file-picker message.
> +let ppmm = Cc['@mozilla.org/parentprocessmessagemanager;1']
> +           .getService(Ci.nsIMessageListenerManager);

nit: align .getService() with ['..']

::: b2g/components/test/mochitest/test_filepicker_path.html
@@ +17,5 @@
> +
> +var testCases = [
> +  // case 1: returns blob with name
> +  { pickedResult: { success: true,
> +                  result: {

nit: indentation is wrong (align 'success' and 'result')

@@ +65,5 @@
> +                            name: 'test5.txt'
> +                          }
> +                },
> +    fileName: 'test5.txt'},
> +  // case 6: returns file without name. This case may be failed because we

nit: This case may fail

@@ +120,5 @@
> +    var name = activeTestCase.pickedResult.result.name;
> +    name = name.replace('/', '\\');
> +    if (name.startWidth('\\')) {
> +      name = 'C:' + name;
> +    }

hm, this windows specific part is a bit ugly. Can you check if other tests do something else in these situations?

Comment 20

[John Hu [:johnhu][:johu][:醬糊小弟]]

Thanks for your reviews, Fabrice.

As to the windows specific part, that's for building the expected result. I had found another one using the same syntax[1], so I use the same way. I don't know if there any other better way. If you know it, please tell me and I will update the patch of it.

I am not sure about one thing: with this patch, the mochitests under b2g/components folder is only enabled in b2g and b2g-debug. So, those tests are not run at b2g-desktop and the windows parts are useless, aren't they?? If yes, I will remove those ugly codes.

Thanks.

[1] http://dxr.mozilla.org/mozilla-central/source/toolkit/content/tests/browser/browser_input_file_tooltips.js#22

Comment 21

[[:fabrice] Fabrice Desré]

Ok. Your tests will run on b2g-desktop, so keep the windows part. b2g/b2g-debug just means "opt build"/"debug build", but they will run on desktop.

Comment 22

[John Hu [:johnhu][:johu][:醬糊小弟]]

Attachment: bug-949944-v6.patch

This patch only fixes the nits mentioned in comment 19. Move r+ to this one.

Comment 23

[John Hu [:johnhu][:johu][:醬糊小弟]]

Attachment: bug-949944-v7.patch

conflict found while testing in the try server. this is the rebased version.

Comment 24

[John Hu [:johnhu][:johu][:醬糊小弟]]

Attachment: bug-949944-v7.patch

Comment 25

[Ethan Tseng [:ethan]]

The result of try server:
https://tbpl.mozilla.org/?tree=Try&rev=3339103da9ea

Comment 26

[Andrew Overholt [:overholt]]

This doesn't seem like something we'd hold 1.3 for but please request aurora uplift approval when ready.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/6b4dbc268a68

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6b4dbc268a68