Send to device context and page action menu should say Send to All Devices

VERIFIED FIXED in Firefox 56

Status

()

Firefox
Sync
P2
normal
VERIFIED FIXED
7 months ago
6 months ago

People

(Reporter: rfeeley, Assigned: Jeongkyu Kim, Mentored)

Tracking

({good-first-bug})

55 Branch
Firefox 56
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox56 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

7 months ago
Created attachment 8871882 [details]
send-to-all-devices.png

See attached.

When we list devices that can be sent a page/link/tab from the context or new Photon page action menu, we also include a way to send to all devices.

"All Devices" is ambiguous (and has been misunderstood to mean manage devices)

We should change it to "Send to All Devices" in all menus that use it.

Updated

7 months ago
Mentor: eoger@fastmail.com
Keywords: good-first-bug
Priority: -- → P2
(Reporter)

Comment 1

6 months ago
The string is located here:
browser/chrome/browser/accounts.properties:sendTabToAllDevices.menuitem
(Assignee)

Comment 2

6 months ago
Let me try to fix this.
Is it enough to change the property value from 'All Devices' to 'Send to All Devices'?

Comment 3

6 months ago
Nope, sadly if you do that our translators will miss the change. You have to also change the property name of the string.
Comment hidden (mozreview-request)
(Assignee)

Comment 5

6 months ago
I see. Sorry for my hasty review request. :-)
The what would be good for new property name?

Comment 6

6 months ago
I guess you could go for sendToAllDevices.menuitem or sendTabToAllDevices.menuitem.label, it's all about creativity here :)
Comment hidden (mozreview-request)
(Assignee)

Comment 8

6 months ago
The sendToAllDevices.menuitem looks good for me. This is my second day of working on Firefox, so it will take a while until my creativity comes into play. :)

Comment 9

6 months ago
mozreview-review
Comment on attachment 8875964 [details]
Bug 1368145 - Change 'All Devices' to 'Send to All Devices' with its property name

https://reviewboard.mozilla.org/r/147370/#review151834

Awesome thanks!
Attachment #8875964 - Flags: review?(eoger) → review+

Comment 10

6 months ago
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1efa46c863a0
Change 'All Devices' to 'Send to All Devices' with its property name r=eoger
Backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=105869425&repo=autoland

Looks like an easy fix, at least. :)

https://hg.mozilla.org/integration/autoland/rev/d5c96e74c92039b8d34cf467b60c4e7e32b44858
Flags: needinfo?(eoger)
Ah, my bad, seems like there's a bit more work for you Jeongkyu :)

The two places you should modify are:

http://searchfox.org/mozilla-central/source/browser/base/content/test/urlbar/browser_page_action_menu.js#280
http://searchfox.org/mozilla-central/source/browser/base/content/test/general/browser_visibleTabs_contextMenu.js#33
Flags: needinfo?(eoger)
(Assignee)

Comment 13

6 months ago
Got it. I will fix them.
By the way, is it OK to create additional commit and submit it using hg review?
You should amend your previous commit instead.
(Assignee)

Comment 15

6 months ago
I amended the last commit but got the following error message when pushed the commit. Can you give me a clue?


$ hg push review
...
abort: Review request is submitted or discarded.
You must reopen the review request before it can be updated.
Review requests should only be reopened if your changes have not landed or have
been backed out - file new bugs for follow-up work.
You have to reopen the review request on reviewboard: https://reviewboard.mozilla.org/r/147368/
Comment hidden (mozreview-request)
Let's wait for the tests to pass before landing this again:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=967e0c4c07472da503d6dfbf2c176a3d22b83f1a

Updated

6 months ago
Assignee: nobody → jeongkyu.kim
Status: NEW → ASSIGNED

Comment 19

6 months ago
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 7ed69c023fb2 -d 83296fc07093: rebasing 401401:7ed69c023fb2 "Bug 1368145 - Change 'All Devices' to 'Send to All Devices' with its property name r=eoger" (tip)
merging browser/base/content/browser-sync.js
merging browser/base/content/test/general/browser_visibleTabs_contextMenu.js
merging browser/base/content/test/urlbar/browser_page_action_menu.js
merging browser/locales/en-US/chrome/browser/accounts.properties
warning: conflicts while merging browser/base/content/browser-sync.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/base/content/test/general/browser_visibleTabs_contextMenu.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Sorry about this Jeongkyu, would you mind rebasing your patch?
(Assignee)

Comment 21

6 months ago
Would you guide me on how to rebase and push the patch properly? Below is heads of my repo. 

$ hg heads .
changeset:   363449:9eb9e6b53887
tag:         tip
fxtree:      central
user:        Ben Hearsum <bhearsum@mozilla.com>
date:        Mon Jun 12 09:45:45 2017 -0400
summary:     bug 1359084: update balrog usernames in more mozharness configs. r=asasaki a=release

changeset:   363207:7ed69c023fb2
parent:      363010:f4262773c433
user:        Jeongkyu Kim <jeongkyu.kim@gmail.com>
date:        Fri Jun 09 12:25:41 2017 +0900
summary:     Bug 1368145 - Change 'All Devices' to 'Send to All Devices' with its property name r=eoger

changeset:   362752:971fba033513
user:        Jeongkyu Kim <jeongkyu.kim@gmail.com>
date:        Thu Jun 08 18:45:49 2017 +0900
summary:     Bug 1370645 - Fix sync notification message following platform conventions

Comment 22

6 months ago
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbd4339e2ca1
Change 'All Devices' to 'Send to All Devices' with its property name. r=eoger
Sorry, this had to be backed out for failing browser-chrome test browser_contextmenu_sendpage.js:

https://hg.mozilla.org/integration/mozilla-inbound/rev/855928e7af3a0b652941edda93fc3e9660c14b5f

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=cbd4339e2ca174a730e8187a9f1c644530098709&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=106750237&repo=mozilla-inbound

[task 2017-06-13T16:21:14.849993Z] 16:21:14     INFO - Entering test bound test_page_contextmenu
[task 2017-06-13T16:21:14.852008Z] 16:21:14     INFO - TEST-PASS | browser/base/content/test/sync/browser_contextmenu_sendpage.js | checking if popup is closed - 
[task 2017-06-13T16:21:14.855669Z] 16:21:14     INFO - TEST-PASS | browser/base/content/test/sync/browser_contextmenu_sendpage.js | Send tab to device is shown - 
[task 2017-06-13T16:21:14.857727Z] 16:21:14     INFO - TEST-PASS | browser/base/content/test/sync/browser_contextmenu_sendpage.js | Send tab to device is enabled - 
[task 2017-06-13T16:21:14.863197Z] 16:21:14     INFO - TEST-PASS | browser/base/content/test/sync/browser_contextmenu_sendpage.js | Foo target is present - 
[task 2017-06-13T16:21:14.869819Z] 16:21:14     INFO - TEST-PASS | browser/base/content/test/sync/browser_contextmenu_sendpage.js | Bar target is present - 
[task 2017-06-13T16:21:14.874241Z] 16:21:14     INFO - Buffered messages finished
[task 2017-06-13T16:21:14.876348Z] 16:21:14     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/sync/browser_contextmenu_sendpage.js | All Devices target is present - Got Send to All Devices, expected All Devices
[task 2017-06-13T16:21:14.883602Z] 16:21:14     INFO - Stack trace:
[task 2017-06-13T16:21:14.886256Z] 16:21:14     INFO - chrome://mochikit/content/browser-test.js:test_is:998
[task 2017-06-13T16:21:14.891076Z] 16:21:14     INFO - chrome://mochitests/content/browser/browser/base/content/test/sync/browser_contextmenu_sendpage.js:test_page_contextmenu:23
[task 2017-06-13T16:21:14.893134Z] 16:21:14     INFO - Leaving test bound test_page_contextmenu

Please fix the issues and submit an updated patch for review.
Flags: needinfo?(jeongkyu.kim)

Comment 24

6 months ago
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4292808c4963
Change 'All Devices' to 'Send to All Devices' with its property name. r=eoger
We got caught in a rebase storm. Fixed the patch.
Flags: needinfo?(jeongkyu.kim)

Comment 26

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4292808c4963
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56

Comment 27

6 months ago
I have reproduced this bug with Nightly 55.0a1 (2017-05-26) (64-bit) on Windows 7, 64 Bit!

This bug's fix is verified with latest Nightly!

Build ID : 20170614030206

User Agent : Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0


[bugday-20170614]
I have reproduced this bug with Nightly 55.0a1 (2017-05-26) (64-bit) on Ubuntu 16.04 LTS!

This bug's fix is verified with latest Nightly!

Build ID   : 20170614100332

User Agent : Mozilla/5.0 (X11; Linux x86_64; rv:56.0) Gecko/20100101 Firefox/56.0
QA Whiteboard: [bugday-20170614]
As per Comment 27 and Comment 28 , I am marking this bug as Verified Fixed.
status-firefox56: fixed → verified

Updated

6 months ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.