849607 - Defect - Sharing in start screen AND if page has no title, should indicate nothing to share instead of error

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: Shell, defect, P1)

Description

[Kamil Jozwiak [:kjozwiak]]

Several metro applications (such as the Khan Academy app) will display a friendly message of: 
"There is nothing to share right now"

However in Metro Firefox, if you try to share from the start screen it says:
"There was a problem with the data from Nightly"

Steps to reproduce the issue:

1. Load Metro Firefox and ensure you are on the start screen
2. Go to the Charms bar (Windows key + C)
3. Click on the share charm

Actual result:
Unfriendly error is displayed

Expected result:
"Nothing to share" should be displayed

Comment 1

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1.

This was really easy to fix and it looks pretty sloppy since a user just has to open the program then go to share, so fixed it.

Jim the fix does the following:
- Moves the setting of data share package properties to the end (we return early with S_OK if we want to have a no data to share)
- Adds an early return S_OK if trying to share a URL which is not from a known scheme
- Cleans up the text and html sharing a bit to only return text and HTML (not title and URL).

Comment 2

[Jim Mathies [:jimm]]

Comment on attachment 723554 [details] [diff] [review]
Patch v1.

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

::: widget/windows/winrt/MetroContracts.cpp
@@ +339,5 @@
> +    HString schemeHString;
> +    uri->get_SchemeName(schemeHString.GetAddressOf());
> +    unsigned int length;
> +    LPCWSTR scheme = schemeHString.GetRawBuffer(&length);
> +    if (wcscmp(scheme, L"http") && wcscmp(scheme, L"https") &&

Lets null check 'scheme'.

@@ +371,5 @@
>  
> +  // Set these properties at the end.  Otherwise users can get a
> +  // "There was a problem with the data package" error when there
> +  // is simply nothing to share.
> +  props->put_ApplicationName(HStringReference(L"Firefox").Get());

I can't remember, does this need to be localized or is it just some sort of internal app id? If it needs to be localized, lets file a bug on it.

::: widget/windows/winrt/MetroUIUtils.js
@@ +96,5 @@
>    get shareText() {
>      let browserWin = Services.wm.getMostRecentWindow("navigator:browser");
>      let tabBrowser = browserWin.getBrowser();
>      if (browserWin && tabBrowser && tabBrowser.contentWindow) {
> +      let sel = tabBrowser.contentWindow.getSelection();

I don't think this will work with selection in sub frames. Can you test and maybe file a follow up or try to address it here?

Comment 3

[Brian R. Bondy [:bbondy]]

(In reply to Jim Mathies [:jimm] from comment #2)
> Comment on attachment 723554 [details] [diff] [review]
> Patch v1.
> 
> ::: widget/windows/winrt/MetroContracts.cpp
> @@ +339,5 @@
> > +    HString schemeHString;
> > +    uri->get_SchemeName(schemeHString.GetAddressOf());
> > +    unsigned int length;
> > +    LPCWSTR scheme = schemeHString.GetRawBuffer(&length);
> > +    if (wcscmp(scheme, L"http") && wcscmp(scheme, L"https") &&
> 
> Lets null check 'scheme'.


OK.

> @@ +371,5 @@
> >  
> > +  // Set these properties at the end.  Otherwise users can get a
> > +  // "There was a problem with the data package" error when there
> > +  // is simply nothing to share.
> > +  props->put_ApplicationName(HStringReference(L"Firefox").Get());
> 
> I can't remember, does this need to be localized or is it just some sort of
> internal app id? If it needs to be localized, lets file a bug on it.

I think it's only an internal app ID but I'll just use the app name #define here.

> ::: widget/windows/winrt/MetroUIUtils.js
> @@ +96,5 @@
> >    get shareText() {
> >      let browserWin = Services.wm.getMostRecentWindow("navigator:browser");
> >      let tabBrowser = browserWin.getBrowser();
> >      if (browserWin && tabBrowser && tabBrowser.contentWindow) {
> > +      let sel = tabBrowser.contentWindow.getSelection();
> 
> I don't think this will work with selection in sub frames. Can you test and
> maybe file a follow up or try to address it here?

It just returns the first range as text, but this is how it already worked before. I'll post a new bug for that as you suggested.

Comment 4

[Jim Mathies [:jimm]]

Comment on attachment 723554 [details] [diff] [review]
Patch v1.

with changes.

Comment 5

[Brian R. Bondy [:bbondy]]

> It just returns the first range as text, but this is how it already 
> worked before. I'll post a new bug for that as you suggested.

Actually I tried with multiple ranges and it works correctly.
I tried both making a selection, then hitting control and making a second selection
And also selecting 2 vertical cells in a table.
Both have the correct text shared, so no new bug is needed.

Comment 6

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2

Resubmitting for review because of an additional fix as part o the patch.
I noticed on pages that don't have a title there was another "there was a problem with the data package" because we need to set a title for the share.  So in those cases I just set the title of the share to the application display name.  

Also replaced "Firefox" with app name and added null check.

Comment 7

[Jim Mathies [:jimm]]

Comment on attachment 723596 [details] [diff] [review]
Patch v2

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

::: widget/windows/winrt/Makefile.in
@@ +32,5 @@
>  		$(NULL)
>  
> +DEFINES		+= -D_IMPL_NS_WIDGET -DMOZ_UNICODE \
> +		-DMOZ_APP_NAME='L"$(MOZ_APP_NAME)"' \
> +		-DMOZ_APP_DISPLAYNAME='L"$(MOZ_APP_DISPLAYNAME)"' \

I don't think this will change for l10n repacks since the metro code doesn't get rebuilt. So I think this is the same as a hard coded "Firefox". I'm not sure if we localize 'Firefox' on other languages or if we always spell it out using english. Seeking r? from axel to be sure.

Comment 8

[Axel Hecht [:Pike]]

Comment on attachment 723596 [details] [diff] [review]
Patch v2

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

We should use the localized brand name. Similar to what we do with the updater.

Comment 9

[Brian R. Bondy [:bbondy]]

Attachment: Patch v3.

Now use the localized brand name for the title and might as well for app name too.
Removed makefile stuff since we aren't using the defines anymore.

Comment 10

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/347a2732f42c

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/347a2732f42c

Comment 12

[Marco Mucci [:MarcoM]]

Hey Brian, just temporarily reopening to include it as part of iteration #4.

Comment 13

[juan becerra [:juanb]]

Tested on 2013-03-15 using Nightly built from http://hg.mozilla.org/mozilla-central/rev/0f7261e288f2
- Tested using the steps in comment #0 where sharing the Firefox Start page shows a message "There's nothing to share right now."
- Tested using pages with no titles like a bugzilla atachment and in that case it has the name off the application (Nightly) and the string for the link. I could send this via mail.

Comment 14

[Samvedana (:Samvedana)]

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0
Build ID: 20130825030201
Built from http://hg.mozilla.org/mozilla-central/rev/01576441bdc6

WFM
Tested on windows 8 using latest nightly for iteration-12. Followed steps provided in comment0 and got expected result.