Closed Bug 1368145 Opened 2 years ago Closed 2 years ago
Send to device context and page action menu should say Send to All Devices
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.
The string is located here: browser/chrome/browser/accounts.properties:sendTabToAllDevices.menuitem
Let me try to fix this. Is it enough to change the property value from 'All Devices' to 'Send to All Devices'?
Nope, sadly if you do that our translators will miss the change. You have to also change the property name of the string.
I see. Sorry for my hasty review request. :-) The what would be good for new property name?
I guess you could go for sendToAllDevices.menuitem or sendTabToAllDevices.menuitem.label, it's all about creativity here :)
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 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+
Pushed by email@example.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
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
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.
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/
Let's wait for the tests to pass before landing this again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=967e0c4c07472da503d6dfbf2c176a3d22b83f1a
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?
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 <firstname.lastname@example.org> 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 <email@example.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 <firstname.lastname@example.org> date: Thu Jun 08 18:45:49 2017 +0900 summary: Bug 1370645 - Fix sync notification message following platform conventions
Pushed by email@example.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.
Pushed by firstname.lastname@example.org: 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.
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.
You need to log in before you can comment on or make changes to this bug.