Closed Bug 817677 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: Widget: Win32, defect)

x86_64
Windows 8.1
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: bbondy, Assigned: bbondy)

References

Details

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

Attachments

(1 file, 1 obsolete file)

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.
Blocks: 801131
Adding metroi-it2 since I did this task during that iteration
Whiteboard: [ie10-parity][metro-it2]
Attached patch Patch v1 (obsolete) (deleted) — Splinter Review
Attachment #694825 - Flags: review?(jmathies)
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 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+
Attached patch Patch v2 (deleted) — Splinter Review
Thanks for the review, implemented review comments and carried forward r+.
Attachment #694825 - Attachment is obsolete: true
Attachment #694834 - Flags: review+
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]
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
No longer blocks: 801131
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.

Attachment

General

Created:
Updated:
Size: