Closed Bug 1058712 Opened 5 years ago Closed 4 years ago

[e10s] Copy Image of context menu does not work in e10s

Categories

(Firefox :: General, defect)

x86_64
Windows 7
defect
Not set
Points:
5

Tracking

()

VERIFIED FIXED
Firefox 40
Iteration:
40.2 - 27 Apr
Tracking Status
e10s + ---
firefox40 --- verified

People

(Reporter: tetsuharu, Assigned: enndeakin)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 3 obsolete files)

+++ This bug was initially created as a clone of Bug #1058539 +++
No longer depends on: 1058539
No longer blocks: 1058701
No longer blocks: 1058708
Assignee: nobody → mconley
tracking-e10s: --- → +
Attached patch copyimagecommand (obsolete) — Splinter Review
There are two issues here:

1. Make sure that the correct command state is propagated up to the parent. This patch is a work in progress that does that.
2. There is a ClipboardProxy that only supports text, so copying images just fails.
Assignee: mconley → enndeakin
Status: NEW → ASSIGNED
Can you give this a points estimate please?
Iteration: --- → 36.1
Flags: qe-verify?
Flags: firefox-backlog+
The second part of comment 1 above would fall under bug 1071562. The first part is in the patch above but needs some polish, perhaps to remove the extra argument to UpdateCommands that I added.
Points: --- → 5
Depends on: 1071562
Hi Neil, can you mark this bug for QE verification.
Flags: needinfo?(enndeakin)
Flags: qe-verify?
Flags: qe-verify+
Flags: needinfo?(enndeakin)
Iteration: 36.1 → ---
Attached patch Copy Image Command, v2 (obsolete) — Splinter Review
This is a simpler version that doesn't modify the command updating api. The only interface change is to add a way to set the winddow root popup node. I added it to nsIContentViewerEdit as it relates to the other methods in that interface, although it isn't ideal.

Once dependent bug 1071562 is fixed, this should work.
Attachment #8493121 - Attachment is obsolete: true
Duplicate of this bug: 1126910
Attached patch Update patch (obsolete) — Splinter Review
This patch removes some unneccesary bits and fixes an issue where <input> elements were treated as images as they implement nsIImageLoadingContent (for <input type="image")
Attachment #8518947 - Attachment is obsolete: true
Attachment #8576691 - Flags: review?(mconley)
Oh and this patch needs both 1071562 which in turn needs 936092.
Iteration: --- → 39.2 - 23 Mar
Unfortunately, I can't reliably test this patch - the patches in bug 936092 and bug 1071562 are pretty bitrotted. So I'll just be doing inspection here.
Comment on attachment 8576691 [details] [diff] [review]
Update patch

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

Hey Enn,

Just a few notes / questions. Thanks!

-Mike

::: browser/base/content/content.js
@@ +172,5 @@
>        spellInfo =
>          InlineSpellCheckerContent.initContextMenu(event, editFlags, this);
>      }
>  
> +    // Update the state of the commands on the context menu. Set the event target

I think this comment should be phrased differently - first, we set the event target so that the copy image command can use it, and _then_ we update the contentcontextmenu commands so that the change to copy image is reflected. Right?

::: browser/base/content/nsContextMenu.js
@@ +455,5 @@
>      // Copy image contents depends on whether we're on an image.
> +    let allowCopyImage;
> +    if (this.isRemote) {
> +      let controller = this.browser.controllers.getControllerForCommand("cmd_copyImage");
> +      allowCopyImage = controller ?

Isn't this just:

allowCopyImage = controller && controller.isCommandEnabled("cmd_copyImage");  ?

Also, what is the case where controller.isCommandEnabled("cmd_copyImage") is false, but this.onImage is true?

::: docshell/base/nsIContentViewerEdit.idl
@@ +31,5 @@
>  	readonly attribute boolean canGetContents;
> +
> +  // Set the node that will be the subject of the editing commands above.
> +  // Usually this will be the node that was context-clicked.
> +  void setCommandNode(in nsIDOMNode aNode);

Nit - might as well carry on the wacky alignment in this file.

::: layout/base/nsDocumentViewer.cpp
@@ +2707,5 @@
>    *aCanGetContents = nsCopySupport::CanCopy(mDocument);
>    return NS_OK;
>  }
>  
> +NS_IMETHODIMP nsDocumentViewer::SetCommandNode(nsIDOMNode* aNode)

You might need a layout peer to review this stuff.

@@ +2714,5 @@
> +  NS_ENSURE_TRUE(document, NS_ERROR_FAILURE);
> +
> +  nsCOMPtr<nsPIDOMWindow> window(document->GetWindow());
> +  NS_ENSURE_TRUE(window, NS_ERROR_NOT_AVAILABLE);
> +  if (window) {

Why if (window)? Line 2717 ensures that it is truthy, otherwise we would have returned NS_ERROR_NOT_AVAILABLE.

@@ +2716,5 @@
> +  nsCOMPtr<nsPIDOMWindow> window(document->GetWindow());
> +  NS_ENSURE_TRUE(window, NS_ERROR_NOT_AVAILABLE);
> +  if (window) {
> +    nsCOMPtr<nsPIWindowRoot> root = window->GetTopWindowRoot();
> +    NS_ENSURE_TRUE(root, NS_ERROR_FAILURE);

This is just NS_ENSURE_STATE(root); no?
Attachment #8576691 - Flags: review?(mconley) → review-
Iteration: 39.2 - 23 Mar → 39.3 - 30 Mar
Attached patch Updated patchSplinter Review
I also added a test.
Attachment #8576691 - Attachment is obsolete: true
Attachment #8584618 - Flags: review?(mconley)
Attachment #8584618 - Flags: review?(ehsan)
Try build is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ef60012a4a1
if you want to try it out without working out the dependencies.
Comment on attachment 8584618 [details] [diff] [review]
Updated patch

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

::: layout/base/nsDocumentViewer.cpp
@@ +3545,5 @@
>    NS_ENSURE_TRUE(node, NS_ERROR_FAILURE);
>  
> +  // Make sure there is a URI assigned. This allows <input type="image"> to
> +  // be an image but rejects other <input> types. This matches what
> +  // nsContentMenu.js does.

Nit: nsContextMenu.js.

Please add a comment to nsContextMenu to remind people to update this logic if they ever change the other side.
Attachment #8584618 - Flags: review?(ehsan) → review+
Comment on attachment 8584618 [details] [diff] [review]
Updated patch

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

::: browser/base/content/content.js
@@ +175,5 @@
>      }
>  
> +    // Set the event target first as the copy image command needs it to
> +    // determine what was context-clicked on. Then, update the state of the
> +    // commands on the context menu. 

Nit - trailing ws

::: browser/base/content/test/general/browser_clipboard.js
@@ +145,5 @@
> +    var doc = content.document;
> +    var main = doc.getElementById("main");
> +    main.focus();
> +
> +    yield new content.Promise((resolve, reject) => {

Nit - I'm pretty sure Promise is available in the ContentTask's scope, without needing to grab it from content.

@@ +154,5 @@
> +        // DataTransfer doesn't support the image types yet, so only text/html
> +        // will be present.
> +        if (clipboardData.getData("text/html") !=
> +            '<img id="img" tabindex="1" src="http://example.org/browser/browser/base/content/test/general/moz.png">') {
> +          return "FAIL";

Should this be a rejection?
Attachment #8584618 - Flags: review?(mconley) → review+
Iteration: 39.3 - 30 Mar → 40.1 - 13 Apr
Attached patch Updated patchSplinter Review
Duplicate of this bug: 1153719
Iteration: 40.1 - 13 Apr → 40.2 - 27 Apr
I'm not checking this one in yet due to a test failure only on Mac.
https://hg.mozilla.org/mozilla-central/rev/9b75aac198d0
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Verified fixed on latest Nightly, build ID: 20150423030204.
Status: RESOLVED → VERIFIED
Depends on: 1281762
You need to log in before you can comment on or make changes to this bug.