952456 - Implement non-text/rich text support for B2G clipboard

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Runtime, defect)

Description

[Jan Jongboom [:janjongboom] (inactive)]

In bug 946646 we added a clipboard module to gonk, but it does only text.

Comment 1

[(no longer active)]

We should finish this where we want to ship bug 946646.

Comment 2

[Bruce Huang [:bhuang] (inactive)]

Moving to 1.5 since the UI work won't be ready to use the clipboard.

Comment 3

[Bruce Huang [:bhuang] (inactive)]

Adding to backlog, enabling text first.

Comment 4

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Is this bug still valid?

Comment 5

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Still valid I think. We don't have non-text support (for example: image) for b2g now.

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Update summary to make the bug more specific.

Comment 7

[Boris Chiou [:boris]]

Attachment: [WIP] Part 1: Implement rich text copy/paste on Gonk (v1)

Handle text/html MIME type on gonk/nsClipboard

Comment 8

[Boris Chiou [:boris]]

Attachment: [WIP] Implement rich text copy/paste and image on Gonk (v2)

Handle text/html and image MIME types on gonk/nsClipboard

Comment 9

[Boris Chiou [:boris]]

Attachment: [WIP] Part 2: Add new BrowserElementAPI to support copy image (v1)

Add new BrowserElementAPI to handle copy image

Comment 10

[Boris Chiou [:boris]]

Attachment: [WIP] Part 2: Add new BrowserElementAPI to support copy image (v2)

Remove some debug code

Comment 11

[Boris Chiou [:boris]]

Attachment: [WIP] Part 2: Add new BrowserElementAPI to support copy image (v2)

Comment 12

[Boris Chiou [:boris]]

Attachment: [WIP] Part 2: Add new BrowserElementAPI to support copy image (v2)

Comment 13

[Boris Chiou [:boris]]

Attachment: [WIP] Part 1: Implement gonk/nsClipboard for rich text and raw image (v3)

Handle text/html and image MIME types on gonk/nsClipboard

Comment 14

[Boris Chiou [:boris]]

Attachment: [WIP] Part 2: Add new BrowserElementAPI to support copy image (v3)

Comment 15

[Boris Chiou [:boris]]

Attachment: CopyImage test app

This app has some local images and hyperlinks, and a content editable area where you can paste your image and hyperlinks.

Comment 16

[Boris Chiou [:boris]]

Attachment: Flow chart - Copy Image

1. Implement new BrowserElement API - CopyImage()
2. Add cmd_copyImage into COMMAND_MAP
3. Set HTMLImageElement when long pressing
4. Implement gonk/nsClipboard.cpp for kHTMLMime, kNativeImageMime, and other image formats.
5. Add new class, mozilla::GonkClipboardData, to save clipboard data on Gonk

Comment 17

[Boris Chiou [:boris]]

Attachment: Flow chart - PDF file

Comment 18

[Boris Chiou [:boris]]

Attachment: Part 1: Implement gonk/nsClipboard for rich text and raw image (v4)

Handle text/html and image MIME types on gonk/nsClipboard

Comment 19

[Boris Chiou [:boris]]

Attachment: Part 2: Add new BrowserElementAPI to support copy image (v4)

Add CopyImage() into BrowserElementAPI

Comment 20

[Boris Chiou [:boris]]

Comment on attachment 8638433 [details] [diff] [review]
Part 1: Implement gonk/nsClipboard for rich text and raw image (v4)

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

Hi froydnj,

I implement nsClipboard::GetData() & nsClipboard::SetData() for text/html and image/[png|jpeg|jpg|gif] mime types, and add a new class, GonkClipboardData, to store the clipboard data on gonk.

Could you please review this patch? Thanks.

Comment 21

[Nathan Froyd [:froydnj]]

Comment on attachment 8638433 [details] [diff] [review]
Part 1: Implement gonk/nsClipboard for rich text and raw image (v4)

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

Thanks for the patch, Boris.  I've just been the reviewer for some refactoring commits, so I don't think I'm the right person to review this new feature.

Fabrice looks to be the other primary reviewer, but he's out for the next several days.  Jan, since you wrote the code initially, do you feel comfortable reviewing?

If Jan doesn't want to or isn't able to, :mwu might be another reasonable choice.

Comment 22

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8638434 [details] [diff] [review]
Part 2: Add new BrowserElementAPI to support copy image (v4)

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

Could we implement this as a new system context menu entry? When the context menu is selected, _recvFireCtxCallback should be able to use cmd_copyImage without the new API.

Comment 23

[Jan Jongboom [:janjongboom] (inactive)]

Comment on attachment 8638433 [details] [diff] [review]
Part 1: Implement gonk/nsClipboard for rich text and raw image (v4)

I don't feel comfortable reviewing this. As suggested in Comment 21, forwarding to mwu.

Comment 24

[Boris Chiou [:boris]]

Attachment: [WIP] Part 2: Support copy image in BrowserElement

Comment 25

[Boris Chiou [:boris]]

Attachment: [WIP] Part 2: Support copy image in BrowserElement

Comment 26

[Boris Chiou [:boris]]

Attachment: [WIP] Add "Copy Link" and "Copy Image" option (gaia example)

Provide 'copy link/url' and "copy image" option when long-pressing hyperlinks

Comment 27

[Boris Chiou [:boris]]

(In reply to Kan-Ru Chen [:kanru] from comment #22)
> Comment on attachment 8638434 [details] [diff] [review]
> Part 2: Add new BrowserElementAPI to support copy image (v4)
> 
> Review of attachment 8638434 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Could we implement this as a new system context menu entry? When the context
> menu is selected, _recvFireCtxCallback should be able to use cmd_copyImage
> without the new API.

Hi Kanru,
In attachment 8640395 [details] [diff] [review], I didn't create new browse element API, and reuse contextMenuItemSelected() as the callback function while press "Copy Image" option in the context menu. HTML img tags don't have the ctxMenuId, so I don't have a good method to make it as a special context menu. Please also check the related gaia usage (attachment 8640402 [details] [diff] [review]). Could you please give me some feedback about this? Thank you.

Comment 28

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8640395 [details] [diff] [review]
[WIP] Part 2: Support copy image in BrowserElement

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

In _getSystemCtxMenuData() we already entry for <img> tags. I think you could create the special menu item in _buildMenuObj if there is <img> tag and ignore ctxMenuId.

Comment 29

[Michael Wu [:mwu]]

Comment on attachment 8638433 [details] [diff] [review]
Part 1: Implement gonk/nsClipboard for rich text and raw image (v4)

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

I'm not familiar with the clipboard code. We should probably wait for fabrice.

Comment 30

[Boris Chiou [:boris]]

Attachment: [WIP] Part 2: Support copy image in BrowserElement (v2)

Comment 31

[Boris Chiou [:boris]]

Attachment: [WIP] Add "Copy Link" and "Copy Image" option (gaia example, v2)

Provide 'copy link/url' and "copy image" option when long-pressing hyperlinks

Comment 32

[Boris Chiou [:boris]]

Comment on attachment 8640995 [details] [diff] [review]
[WIP] Part 2: Support copy image in BrowserElement (v2)

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

Hi Kanru, 

How about this implementation? I added "Copy Image" as a special context menu option. Thanks.

Comment 33

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8640995 [details] [diff] [review]
[WIP] Part 2: Support copy image in BrowserElement (v2)

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

Looks good.

::: dom/browser-element/BrowserElementChildPreload.js
@@ +868,5 @@
> +
> +      // Enable copy image option
> +      if (!hasImgElement && elem.nodeName == 'IMG') {
> +        hasImgElement = true;
> +      }

!hasImgElement is always true

Comment 34

[GitHub Autolander Bot]

Attachment: [gaia] BorisChiou:Bug952456 > mozilla-b2g:master

Comment 35

[Boris Chiou [:boris]]

Attachment: Part 2: Support copy image in BrowserElement (v3)

Comment 36

[Boris Chiou [:boris]]

(In reply to Kan-Ru Chen [:kanru] from comment #33)
> Comment on attachment 8640995 [details] [diff] [review]
> [WIP] Part 2: Support copy image in BrowserElement (v2)
> 
> Review of attachment 8640995 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good.
> 
> ::: dom/browser-element/BrowserElementChildPreload.js
> @@ +868,5 @@
> > +
> > +      // Enable copy image option
> > +      if (!hasImgElement && elem.nodeName == 'IMG') {
> > +        hasImgElement = true;
> > +      }
> 
> !hasImgElement is always true

I removed this check in the newer patch. Thanks for your feedback :)

Comment 37

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8641457 [details] [diff] [review]
Part 2: Support copy image in BrowserElement (v3)

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

::: dom/browser-element/BrowserElementChildPreload.js
@@ +1250,5 @@
> +      menuObj.items.push({id: 'copy-image'});
> +    } else if (!menu) {
> +      // menu is null and hasImgElement is false
> +      return null;
> +    }

return null changes behavior. Originally we always return menuObj which may or may not have zero items and I think we should still do this.

Comment 38

[Boris Chiou [:boris]]

(In reply to Kan-Ru Chen [:kanru] from comment #37)
> Comment on attachment 8641457 [details] [diff] [review]
> Part 2: Support copy image in BrowserElement (v3)
> 
> Review of attachment 8641457 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/browser-element/BrowserElementChildPreload.js
> @@ +1250,5 @@
> > +      menuObj.items.push({id: 'copy-image'});
> > +    } else if (!menu) {
> > +      // menu is null and hasImgElement is false
> > +      return null;
> > +    }
> 
> return null changes behavior. Originally we always return menuObj which may
> or may not have zero items and I think we should still do this.

Sure, I will upload a new one to fix this part. Thanks.

Comment 39

[Boris Chiou [:boris]]

Attachment: Part 2: Support copy image in BrowserElement (v4, carry kchen's r+)

Add a new context menu option, copy image.

Comment 40

[[:fabrice] Fabrice Desré]

Comment on attachment 8638433 [details] [diff] [review]
Part 1: Implement gonk/nsClipboard for rich text and raw image (v4)

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

That looks fine, I'm setting f+ for now just because I'd like to know about the mImgTool use.

::: widget/gonk/GonkClipboardData.cpp
@@ +47,5 @@
> +
> +void
> +GonkClipboardData::SetImage(gfx::DataSourceSurface* aDataSource)
> +{
> +  // Clone a new DataSourceSurface and store it

nit: add a full stop at the end of comments. (not just this one)

::: widget/gonk/nsClipboard.h
@@ +1,2 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* vim:set ts=2 sw=2 sts=2 et: */

let's not fight on the mode-line please :)

@@ +26,5 @@
>    ~nsClipboard() {}
> +
> +private:
> +  mozilla::UniquePtr<mozilla::GonkClipboardData> mClipboard;
> +  nsCOMPtr<imgITools> mImgTool;

I'm not sure how costly it is to recreate an instance, but if it's cheap I don't see any good reason to tie that to nsClipboard.

Comment 41

[[:fabrice] Fabrice Desré]

Also, I'd really like to see tests.

Comment 42

[Boris Chiou [:boris]]

Comment on attachment 8638433 [details] [diff] [review]
Part 1: Implement gonk/nsClipboard for rich text and raw image (v4)

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

::: widget/gonk/GonkClipboardData.cpp
@@ +47,5 @@
> +
> +void
> +GonkClipboardData::SetImage(gfx::DataSourceSurface* aDataSource)
> +{
> +  // Clone a new DataSourceSurface and store it

OK.

::: widget/gonk/nsClipboard.h
@@ +1,2 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* vim:set ts=2 sw=2 sts=2 et: */

OK :)

@@ +26,5 @@
>    ~nsClipboard() {}
> +
> +private:
> +  mozilla::UniquePtr<mozilla::GonkClipboardData> mClipboard;
> +  nsCOMPtr<imgITools> mImgTool;

I think it wouldn't cost too much according to imgITools.h. I will make it as a local variable.

Comment 43

[Boris Chiou [:boris]]

(In reply to [:fabrice] Fabrice Desré from comment #41)
> Also, I'd really like to see tests.
OK.

I wrote a gaia marionette test for "copy image" in attachment 8644902 [details] [diff] [review] (Bug 1121463) already, and will write a mochitest to test nsClipboard::SetData()/GetData() to make sure that this module works on Firefox OS.

Thanks for your review.

Comment 44

[Boris Chiou [:boris]]

Attachment: Part 1: Implement gonk/nsClipboard for rich text and raw image (v5)

Handle text/html and image MIME types on gonk/nsClipboard

Revised according to fabrice's review.

Comment 45

[Boris Chiou [:boris]]

Attachment: Part 2: Support copy image in BrowserElement (v4, carry kchen's r+)

Add a new context menu option, copy image.

Rebased.

Comment 46

[Boris Chiou [:boris]]

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

Comment 47

[Boris Chiou [:boris]]

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

Comment 48

[Boris Chiou [:boris]]

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

Comment 49

[Boris Chiou [:boris]]

Attachment: Part 1: Implement gonk/nsClipboard for rich text and raw image (v6)

Handle text/html and image MIME types on gonk/nsClipboard

Fix HasDataMatchingFlavors() for my mochitest.

Comment 50

[Boris Chiou [:boris]]

Attachment: Part 2: Support copy image in BrowserElement (v4, carry kchen's r+)

Add a new context menu option, copy image.

Rebased.

Comment 51

[Boris Chiou [:boris]]

Attachment: Part 3: Enable test_copyimage.html on gonk/cocoa (v1)

Enable dom/base/test/test_copyimage.html on b2g emulators and macos

Comment 52

[Boris Chiou [:boris]]

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

Comment 53

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris] from comment #51)
> Created attachment 8646839 [details] [diff] [review]
> Part 3: Enable test_copyimage.html on gonk/cocoa (v1)
> 
> Enable dom/base/test/test_copyimage.html on b2g emulators and macos

I also enable test_copyimage.html on cocoa to avoid other bugs like Bug 1181467.

Comment 54

[Boris Chiou [:boris]]

Attachment: Part 3: Enable test_copyimage.html on gonk/cocoa (v2)

Enable dom/base/test/test_copyimage.html on b2g emulators and macos

Comment 55

[Boris Chiou [:boris]]

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

Comment 56

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/e00a383520f6
https://hg.mozilla.org/integration/b2g-inbound/rev/3e5d45dcd5c6
https://hg.mozilla.org/integration/b2g-inbound/rev/df6e7f025c65

Comment 57

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

https://hg.mozilla.org/integration/b2g-inbound/rev/e00a383520f6
https://hg.mozilla.org/integration/b2g-inbound/rev/3e5d45dcd5c6
https://hg.mozilla.org/integration/b2g-inbound/rev/df6e7f025c65

Comment 58

[Nigel Babu [:nigelb]]

This has caused perma fails in B2G ICS Emulator opt Mochitest Mochitest M(2)

11. '/tools/buildbot/bin/python scripts/scripts/b2g_emulator_unittest.py ...' warnings 51m 44s 31ms
3213 21:42:58 INFO - 578 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | paste command works (test: content editable div)
3215 21:43:06 INFO - 579 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | Timed out while polling clipboard for pasted data
3218 21:43:06 INFO - 580 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | cut function works (test: content editable div)
3233 21:43:13 INFO - 590 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | paste command works (test: normal div with designMode:on)
3235 21:43:20 INFO - 591 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | Timed out while polling clipboard for pasted data
3238 21:43:20 INFO - 592 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | cut function works (test: normal div with designMode:on)
3270 21:43:45 INFO - 614 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | paste command works (test: content editable div)
3272 21:43:52 INFO - 615 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | Timed out while polling clipboard for pasted data
3275 21:43:52 INFO - 616 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | cut function works (test: content editable div)
3290 21:44:00 INFO - 626 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | paste command works (test: normal div with designMode:on)
3292 21:44:07 INFO - 627 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | Timed out while polling clipboard for pasted data
3295 21:44:07 INFO - 628 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | cut function works (test: normal div with designMode:on)
3324 21:44:32 INFO - 650 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | paste command works (test: content editable div)
3326 21:44:40 INFO - 651 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | Timed out while polling clipboard for pasted data
3329 21:44:40 INFO - 652 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | cut function works (test: content editable div)
3344 21:44:48 INFO - 662 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | paste command works (test: normal div with designMode:on)
3346 21:44:56 INFO - 663 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | Timed out while polling clipboard for pasted data
3349 21:44:56 INFO - 664 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | cut function works (test: normal div with designMode:on)
3381 21:45:24 INFO - 686 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | paste command works (test: content editable div)
3383 21:45:32 INFO - 687 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | Timed out while polling clipboard for pasted data
3386 21:45:32 INFO - 688 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | cut function works (test: content editable div)
3401 21:45:41 INFO - 698 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | paste command works (test: normal div with designMode:on)
3403 21:45:49 INFO - 699 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | Timed out while polling clipboard for pasted data
3406 21:45:49 INFO - 700 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | cut function works (test: normal div with designMode:on)
3677 21:51:07 INFO - 794 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_oop_CopyPaste.html | paste command works (test: normal div with designMode:on)
3679 21:51:14 INFO - 795 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_oop_CopyPaste.html | Timed out while polling clipboard for pasted data
3682 21:51:14 INFO - 796 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_oop_CopyPaste.html | cut function works (test: normal div with designMode:on)
3724 21:51:46 INFO - 830 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_oop_CopyPaste.html | paste command works (test: normal div with designMode:on)
3726 21:51:53 INFO - 831 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_oop_CopyPaste.html | Timed out while polling clipboard for pasted data
3729 21:51:53 INFO - 832 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_oop_CopyPaste.html | cut function works (test: normal div with designMode:on)
27366 22:01:02 ERROR - 08-18 04:58:40.620 F/libc ( 775): Fatal signal 11 (SIGSEGV) at 0x00000000 (code=1)
27367 22:01:02 ERROR - This usually indicates the B2G process has crashed
27459 22:01:02 ERROR - Return code: 1 

https://treeherder.mozilla.org/logviewer.html#?job_id=2548452&repo=b2g-inbound

Comment 59

[Boris Chiou [:boris]]

(In reply to [e-mail] [:nigelb] from comment #58)
> This has caused perma fails in B2G ICS Emulator opt Mochitest Mochitest M(2)
> 
> 11. '/tools/buildbot/bin/python scripts/scripts/b2g_emulator_unittest.py
> ...' warnings 51m 44s 31ms
> 3213 21:42:58 INFO - 578 INFO TEST-UNEXPECTED-FAIL |
> dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html |
> paste command works (test: content editable div)
> 3215 21:43:06 INFO - 579 INFO TEST-UNEXPECTED-FAIL |
> dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html |
> Timed out while polling clipboard for pasted data
> 3218 21:43:06 INFO - 580 INFO TEST-UNEXPECTED-FAIL |
> dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html |
> cut function works (test: content editable div)
> 3233 21:43:13 INFO - 590 INFO TEST-UNEXPECTED-FAIL |
> dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html |
> paste command works (test: normal div with designMode:on)
> 3235 21:43:20 INFO - 591 INFO TEST-UNEXPECTED-FAIL |
> dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html |
> Timed out while polling clipboard for pasted data
> 3238 21:43:20 INFO - 592 INFO TEST-UNEXPECTED-FAIL |
> dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html |
> cut function works (test: normal div with designMode:on)
> 3270 21:43:45 INFO - 614 INFO TEST-UNEXPECTED-FAIL |
> dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html |
> paste command works (test: content editable div)
> 3272 21:43:52 INFO - 615 INFO TEST-UNEXPECTED-FAIL |
> dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html |
> Timed out while polling clipboard for pasted data
> 3275 21:43:52 INFO - 616 INFO TEST-UNEXPECTED-FAIL |
> dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html |
> cut function works (test: content editable div)
> 3290 21:44:00 INFO - 626 INFO TEST-UNEXPECTED-FAIL |
> dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html |
> paste command works (test: normal div with designMode:on)
> 3292 21:44:07 INFO - 627 INFO TEST-UNEXPECTED-FAIL |
> dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html |
> Timed out while polling clipboard for pasted data
> 3295 21:44:07 INFO - 628 INFO TEST-UNEXPECTED-FAIL |
> dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html |
> cut function works (test: normal div with designMode:on)
> 3324 21:44:32 INFO - 650 INFO TEST-UNEXPECTED-FAIL |
> dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html |
> paste command works (test: content editable div)
> 3326 21:44:40 INFO - 651 INFO TEST-UNEXPECTED-FAIL |
> dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html |
> Timed out while polling clipboard for pasted data
> 3329 21:44:40 INFO - 652 INFO TEST-UNEXPECTED-FAIL |
> dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html |
> cut function works (test: content editable div)
> 3344 21:44:48 INFO - 662 INFO TEST-UNEXPECTED-FAIL |
> dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html |
> paste command works (test: normal div with designMode:on)
> 3346 21:44:56 INFO - 663 INFO TEST-UNEXPECTED-FAIL |
> dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html |
> Timed out while polling clipboard for pasted data
> 3349 21:44:56 INFO - 664 INFO TEST-UNEXPECTED-FAIL |
> dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html |
> cut function works (test: normal div with designMode:on)
> 3381 21:45:24 INFO - 686 INFO TEST-UNEXPECTED-FAIL |
> dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html |
> paste command works (test: content editable div)
> 3383 21:45:32 INFO - 687 INFO TEST-UNEXPECTED-FAIL |
> dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html |
> Timed out while polling clipboard for pasted data
> 3386 21:45:32 INFO - 688 INFO TEST-UNEXPECTED-FAIL |
> dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html |
> cut function works (test: content editable div)
> 3401 21:45:41 INFO - 698 INFO TEST-UNEXPECTED-FAIL |
> dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html |
> paste command works (test: normal div with designMode:on)
> 3403 21:45:49 INFO - 699 INFO TEST-UNEXPECTED-FAIL |
> dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html |
> Timed out while polling clipboard for pasted data
> 3406 21:45:49 INFO - 700 INFO TEST-UNEXPECTED-FAIL |
> dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html |
> cut function works (test: normal div with designMode:on)
> 3677 21:51:07 INFO - 794 INFO TEST-UNEXPECTED-FAIL |
> dom/browser-element/mochitest/test_browserElement_oop_CopyPaste.html | paste
> command works (test: normal div with designMode:on)
> 3679 21:51:14 INFO - 795 INFO TEST-UNEXPECTED-FAIL |
> dom/browser-element/mochitest/test_browserElement_oop_CopyPaste.html | Timed
> out while polling clipboard for pasted data
> 3682 21:51:14 INFO - 796 INFO TEST-UNEXPECTED-FAIL |
> dom/browser-element/mochitest/test_browserElement_oop_CopyPaste.html | cut
> function works (test: normal div with designMode:on)
> 3724 21:51:46 INFO - 830 INFO TEST-UNEXPECTED-FAIL |
> dom/browser-element/mochitest/test_browserElement_oop_CopyPaste.html | paste
> command works (test: normal div with designMode:on)
> 3726 21:51:53 INFO - 831 INFO TEST-UNEXPECTED-FAIL |
> dom/browser-element/mochitest/test_browserElement_oop_CopyPaste.html | Timed
> out while polling clipboard for pasted data
> 3729 21:51:53 INFO - 832 INFO TEST-UNEXPECTED-FAIL |
> dom/browser-element/mochitest/test_browserElement_oop_CopyPaste.html | cut
> function works (test: normal div with designMode:on)
> 27366 22:01:02 ERROR - 08-18 04:58:40.620 F/libc ( 775): Fatal signal 11
> (SIGSEGV) at 0x00000000 (code=1)
> 27367 22:01:02 ERROR - This usually indicates the B2G process has crashed
> 27459 22:01:02 ERROR - Return code: 1 
> 
> https://treeherder.mozilla.org/logviewer.html#?job_id=2548452&repo=b2g-
> inbound

Thanks, I should fix this problem ASAP and checkin again.

Comment 60

[Boris Chiou [:boris]]

(In reply to [e-mail] [:nigelb] from comment #58)
> This has caused perma fails in B2G ICS Emulator opt Mochitest Mochitest M(2)
> 
> https://treeherder.mozilla.org/logviewer.html#?job_id=2548452&repo=b2g-
> inbound

Could you please back out these patches in b2g-inbound? I will fix this problem soon and request check-in needed again. Thanks.

Comment 61

[Boris Chiou [:boris]]

Attachment: Part 1: Implement gonk/nsClipboard for rich text and raw image (v7, carry fabrice's r+)

Handle text/html and image MIME types on gonk/nsClipboard

Fix tests failed

Comment 62

[Boris Chiou [:boris]]

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

Comment 63

[Boris Chiou [:boris]]

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

Comment 64

[Ryan VanderMeulen [:RyanVM]]

Backed out for B2G test_browserElement_inproc_CopyPaste.html failures.
https://treeherder.mozilla.org/logviewer.html#?job_id=2546909&repo=b2g-inbound

https://hg.mozilla.org/integration/b2g-inbound/rev/3ef1a1856a3f

Comment 65

[Ryan VanderMeulen [:RyanVM]]

One other issue this patch encountered:
https://treeherder.mozilla.org/logviewer.html#?job_id=2549898&repo=b2g-inbound

This one wasn't directly your fault. What happened is that the newly-reenabled test_copyimage.html changed the chunking boundaries such that test_CrossSiteXHR_origin.html went from the end of M(16) to the beginning of M(17). This caused the already-slow test to be even more slowed-down by the emulator startup to the point where it was timing out 90+% of the time. I've since pushed a test change to request a longer timeout to hopefully avoid this problem in the future (since we're effectively one test away from hitting it again on any other branch too), but you'll need to rebase your patch on top of mozilla-central tip to find out.

Please do that and push to Try again. |try: -b d -p emulator -u mochitests -t none| will give you a targeted run for only B2G emulator debug mochitests.

Comment 66

[Ryan VanderMeulen [:RyanVM]]

Looks like the Try push from comment 63 is still hitting the test_browserElement_inproc_CopyPaste.html failures it was originally backed out for anyway.

Comment 67

[Boris Chiou [:boris]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #65)
> One other issue this patch encountered:
> https://treeherder.mozilla.org/logviewer.html#?job_id=2549898&repo=b2g-
> inbound
> 
> This one wasn't directly your fault. What happened is that the
> newly-reenabled test_copyimage.html changed the chunking boundaries such
> that test_CrossSiteXHR_origin.html went from the end of M(16) to the
> beginning of M(17). This caused the already-slow test to be even more
> slowed-down by the emulator startup to the point where it was timing out
> 90+% of the time. I've since pushed a test change to request a longer
> timeout to hopefully avoid this problem in the future (since we're
> effectively one test away from hitting it again on any other branch too),
> but you'll need to rebase your patch on top of mozilla-central tip to find
> out.
> 
> Please do that and push to Try again. |try: -b d -p emulator -u mochitests
> -t none| will give you a targeted run for only B2G emulator debug mochitests.

Ok, I will Try again after fixing test_browserElement_inproc_CopyPaste.html. Thanks for your reminder.

Comment 68

[Boris Chiou [:boris]]

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

Comment 69

[Boris Chiou [:boris]]

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

Comment 70

[Boris Chiou [:boris]]

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

Comment 71

[Boris Chiou [:boris]]

Attachment: Part 1: Implement gonk/nsClipboard for rich text and raw image. (v8, carry fabrice's r+)

Handle text/html and image MIME types on gonk/nsClipboard.

A minor fix for test_browserElement_inproc_CopyPaste.html.
Note: Call EmptyClipboard() first in nsClipboard::SetData();

Comment 72

[Boris Chiou [:boris]]

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

Comment 73

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris] from comment #72)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=00afeda8597c

Looks like pass all the mochitests.

Comment 74

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/0bd5e39bb91e
https://hg.mozilla.org/integration/b2g-inbound/rev/cc9bc0e573fd
https://hg.mozilla.org/integration/b2g-inbound/rev/1c1b9bf94e4e

Comment 75

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0bd5e39bb91e
https://hg.mozilla.org/mozilla-central/rev/cc9bc0e573fd
https://hg.mozilla.org/mozilla-central/rev/1c1b9bf94e4e

Comment 76

[Boris Chiou [:boris]]

Could anyone merge my gaia pull-request? Thank you.

Comment 77

[Boris Chiou [:boris]]

Hi, Tim

Looks like the implementation in gecko side was merged. Could you please merge the gaia pull request?
Thank you.

Comment 78

[Tim Guan-tin Chien [:timdream] (please needinfo)]

master: https://github.com/mozilla-b2g/gaia/commit/22fba58ea99fc8212de8d4d44f366773b439dd20

Comment 79

[Julien Wajsberg [:julienw]]

I'm really upset because it's the first time I hear about this feature being done for 2.5. Maybe it's me and I just missed a mail, but I think whoever was responsible for this feature should have been a little more proactive and look in Gaia _in advance_ to see what could break, so that we could plan ahead.

So now, big surprise, it's breaking the SMS app. The main reason is that we use contenteditable in a lot of places, and we try to control what's happening in it.

I'm sure the SMS app is not the only app to be broken by the change and the lack of planning. I think we should put the functionality behind a pref for 2.5, because it's already quite late in the 2.5 cycle. I don't think we should backout, but just hide behind a pref, so that it's easy to find and fix the issues we have, and plan for a post-2.5 schedule.

Comment 80

[Francisco Jordano [:arcturus] [:francisco]]

I agree with Julien here, perhaps I missed an email to the mailing list, but is the first time I heard about this feature.

Right now we are having problems with this feature in SMS:
https://bugzilla.mozilla.org/show_bug.cgi?id=1207083
https://bugzilla.mozilla.org/show_bug.cgi?id=1209900

Which makes the SMS app broken, we need to check if other apps like email, contacts, etc. are also affected by this.

I do agree with Julien, we should add a pref, we are getting close to the end of feature landing, and we are adding pressure to other teams having their apps broken.

Comment 81

[[:fabrice] Fabrice Desré]

No one is sending emails each time they land something. Be realistic there guys and assume the best.

It's unfortunate that it broke the sms app, but now we have options:
- figure out what it takes to make that work in the sms app (I think it's nice to copy/paste images to send MMS for instance!)
- disable that in 2.5 with a pref.

Boris, can you get a patch ready with a pref toggle? We'll let it on in m-c and will turn it off if needed once we branch 2.5

Comment 82

[James Burke [:jrburke]]

I tried the steps in bug 1209900 to copy an image from a google images search page, then paste that in an email Compose message body.

Email uses contenteditable for the compose body, but currently only handles text.

The pasting worked, in that I could see it in the compose body. However, the blinking cursor display was over the image, and pressing Enter in this state created new lines above the image. The image filled the width of the compose, maybe that has something to do with the cursor behavior.

The Send did not produce an error in the log, but the received message was only the text I put in the body, no image. I believe this is expected, as the email back end only assumes text-only sends at the moment

Cc'ing asuth who can provide more background if needed, but end result: email definitely would need at least one bug filed if rich text pasting is expected to work there. Given where 2.5 is at on its timeline, I expect this would be a post-2.5 fix.

Comment 83

[Andrew Sutherland [:asuth] (he/him)]

I think for the email app (and I guess SMS too?) we want to just add our own explicit "paste" handle, prevent the default paste, and explicitly insert the plaintext ourselves.  This makes the limitations clear to the user, although it may make them sad/frustrated and saves us from the many complicated privacy and problem domain issues.  (See http://stackoverflow.com/questions/12027137/javascript-trick-for-paste-as-plain-text-in-execcommand for example implementations of this.)

We can then spin-off bugs as appropriate to get fancy.  For example, email has to deal with the whole multipart/mixed and cid protocol issues if we're doing actual ininline stuff.  Plus there's potential sanitization, etc.  For now the only viable use-case for the user sharing an image into the email app is to use the browser to generate a "share" activity.

Comment 84

[Andrew Sutherland [:asuth] (he/him)]

Uh, noting that in the stackoverflow examples, we would absolutely want to be using "insertText" if using execCommand and not "insertHTML".

Comment 85

[James Burke [:jrburke]]

I filed bug 1210151 to track just pasting the plain text in for email, will have a pull request up shortly.

Comment 86

[Boris Chiou [:boris]]

(In reply to [:fabrice] Fabrice Desré from comment #81)
> No one is sending emails each time they land something. Be realistic there
> guys and assume the best.
> 
> It's unfortunate that it broke the sms app, but now we have options:
> - figure out what it takes to make that work in the sms app (I think it's
> nice to copy/paste images to send MMS for instance!)
> - disable that in 2.5 with a pref.
> 
> Boris, can you get a patch ready with a pref toggle? We'll let it on in m-c
> and will turn it off if needed once we branch 2.5

Sure. I created Bug 1210265 to handle this case.

Comment 87

[Francisco Jordano [:arcturus] [:francisco]]

(In reply to Boris Chiou [:boris] from comment #86)
> (In reply to [:fabrice] Fabrice Desré from comment #81)
> > No one is sending emails each time they land something. Be realistic there
> > guys and assume the best.

I would usually agree with that comment, but not in cases for features that could potentially break multiple apps.

Anyway, agree again, let's stop whining and get this shorted :)

> > 
> > It's unfortunate that it broke the sms app, but now we have options:
> > - figure out what it takes to make that work in the sms app (I think it's
> > nice to copy/paste images to send MMS for instance!)
> > - disable that in 2.5 with a pref.

Sounds perfect, having the pref, release the pressure for 2.5 and we can plan how to integrate this.

> > 
> > Boris, can you get a patch ready with a pref toggle? We'll let it on in m-c
> > and will turn it off if needed once we branch 2.5
> 
> Sure. I created Bug 1210265 to handle this case.

\o/ Thanks a lot!