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)
Tracking
()
RESOLVED
FIXED
mozilla20
People
(Reporter: bbondy, Assigned: bbondy)
References
Details
(Whiteboard: [ie10-parity][metro-it2][completed-elm])
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
bbondy
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Comment 1•12 years ago
|
||
Adding metroi-it2 since I did this task during that iteration
Whiteboard: [ie10-parity][metro-it2]
Assignee | ||
Comment 2•12 years ago
|
||
Attachment #694825 -
Flags: review?(jmathies)
Assignee | ||
Comment 3•12 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•12 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•12 years ago
|
||
Thanks for the review, implemented review comments and carried forward r+.
Attachment #694825 -
Attachment is obsolete: true
Attachment #694834 -
Flags: review+
Assignee | ||
Comment 6•12 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•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Updated•10 years ago
|
OS: Windows 8 Metro → Windows 8.1
You need to log in
before you can comment on or make changes to this bug.
Description
•