Sharing in Firefox Metro should only share selected content when selection is made

RESOLVED FIXED in mozilla20

Status

()

Core
Widget: Win32
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: bbondy, Assigned: bbondy)

Tracking

unspecified
mozilla20
x86_64
Windows 8.1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ie10-parity][metro-it2][completed-elm])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
When the user selects a part of a web page, only that part should be provided in the share contract in both HTML and text formats.
(Assignee)

Updated

6 years ago
Blocks: 801131
(Assignee)

Comment 1

6 years ago
Adding metroi-it2 since I did this task during that iteration
Whiteboard: [ie10-parity][metro-it2]
(Assignee)

Comment 2

6 years ago
Created attachment 694825 [details] [diff] [review]
Patch v1
Attachment #694825 - Flags: review?(jmathies)
(Assignee)

Comment 3

6 years ago
Comment on attachment 694825 [details] [diff] [review]
Patch v1

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

The IDL review is for m-c by the way, the rest is for elm only for now.

Comment 4

6 years ago
Comment on attachment 694825 [details] [diff] [review]
Patch v1

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

::: widget/MetroUIUtils.idl
@@ +19,5 @@
>    */
> +  attribute AString currentPageURI;
> +
> +  /**
> +   * Obtains the current page title 

nit - a couple trailing white space issues here in splinter diff.

::: widget/windows/winrt/MetroContracts.cpp
@@ +312,5 @@
> +  if (NS_FAILED(rv) || NS_FAILED(rv2)) {
> +    return E_FAIL;
> +  }
> +
> +  // Get the package tto share and initialize it

nit - tto

@@ +335,5 @@
> +
> +  // Add whatever content metroUIUtils wants us to for the text sharing
> +  nsString shareText;
> +  if (NS_SUCCEEDED(metroUIUtils->GetShareText(shareText)))
> +  {

nit - brace folding

@@ +338,5 @@
> +  if (NS_SUCCEEDED(metroUIUtils->GetShareText(shareText)))
> +  {
> +    AssertRetHRESULT(hr = dataPackage->SetText(HStringReference(shareText.BeginReading()).Get()) ,hr);
> +  }
> +  

nit - white space

::: widget/windows/winrt/MetroUIUtils.js
@@ +32,5 @@
>        let browserWin = Services.wm.getMostRecentWindow("navigator:browser");
> +      let tabBrowser = browserWin.getBrowser();
> +      if (!browserWin || !tabBrowser || !tabBrowser.contentWindow ||
> +          !tabBrowser.contentWindow.getSelection() ||
> +          tabBrowser.contentWindow.getSelection().toString().length == 0) {

I think getSelection might be expensive, maybe store the result in a variable and check that instead. Also, !val vs. val == 0.

@@ +42,5 @@
>      }
> +  },
> +
> +  /**
> +   * Obtains the current page title 

nit - white space
Attachment #694825 - Flags: review?(jmathies) → review+
(Assignee)

Comment 5

6 years ago
Created attachment 694834 [details] [diff] [review]
Patch v2

Thanks for the review, implemented review comments and carried forward r+.
Attachment #694825 - Attachment is obsolete: true
Attachment #694834 - Flags: review+
(Assignee)

Comment 6

6 years ago
Slight mod from patch v2 as I seen another case with 2 getSelection() calls. 

https://hg.mozilla.org/projects/elm/rev/30479ef6ff85

With push the idl to m-c once it is not a closed tree.
Whiteboard: [ie10-parity][metro-it2] → [ie10-parity][metro-it2][completed-elm]
(Assignee)

Comment 7

6 years ago
https://hg.mozilla.org/mozilla-central/rev/cf15d495a7fd
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20

Updated

5 years ago
No longer blocks: 801131

Updated

5 years ago
Blocks: 833123
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.