For the toolbar icon, we currently have the label as "Loop" (shown when customizing the icon), and the tooltip text as "Start a conversation". Are we happy with that for MLP or do we want something different? I suspect Darrin/Romain need to answer this. Once we have some text, then we can hook it up to l10n for our nightly users.

Here are my suggestions * Button Tooltip: "Invite someone to talk" * Panel text: "Get a link to invite someone to talk" (trying to be consistent with Phase 1 UX although currently you have to request a link therefore the language is slightly different) Darrin is that OK with you?

Sounds reasonable Romain. I wonder if "Get a link *and* invite someone to talk" is better? Pretty much the same thing, just avoids using 'to' twice.

I'm good with that!

Split out naming of the icon to bug 1013989.

This fixes this bug and bug 1005065 (they were both simple, so one patch seems best). We've been told it is ok to add the loop button by default for Nightly. The label hasn't been decided on yet, but we don't want to expose "Loop" there, so we'll leave it blank for MLP and fix it in bug 1013989. The loop.properties change of "get_link_to_share" doesn't need an id change as we haven't yet landed this in a repository where L10n is translating.

Screenshot of the new text & tooltip from the patch.

Comment on attachment 8426270 [details] [diff] [review] Show the Loop button by default and update the tooltip and panel text. Review of attachment 8426270 [details] [diff] [review]: ----------------------------------------------------------------- Jaws or Gijs, can one of you please have a quick look at this? It looks fine to me, but I know almost nothing about CustomizableUI or default-set

Comment on attachment 8426270 [details] [diff] [review] Show the Loop button by default and update the tooltip and panel text. Review of attachment 8426270 [details] [diff] [review]: ----------------------------------------------------------------- This is completely sane as far as code goes (assuming you've verified that MOZ_LOOP is appropriately defined in all the right places), but you can't change a string (in loop.properties) and not rev the string ID. (not even entirely sure what that hunk is doing in this patch - it seems unrelated)

Comment on attachment 8426270 [details] [diff] [review] Show the Loop button by default and update the tooltip and panel text. Gijs seems correct that the loop.properties chunk shouldn't be in this patch - it should be in the loop-landing patch. Given that file isn't already in the tree (and assuming you haven't had l10n love on this yet), it's *probably* OK to keep the same string ID - but as soon as localizers have been let loose on it, it would not be OK to reuse the same ID. I'm still somewhat concerned about the landing process - we are being asked to review code that is not yet in the tree, but is based on stuff in some git repo somewhere. But that's how loop is rolling and I trust you to keep the "landing branch" current with only reviewed patches, so a somewhat reluctant r+...

Comment on attachment 8426270 [details] [diff] [review] Show the Loop button by default and update the tooltip and panel text. Review of attachment 8426270 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/customizableui/src/CustomizableUI.jsm @@ +200,5 @@ > "bookmarks-menu-button", > "downloads-button", > "home-button", > +#ifdef MOZ_LOOP > + "loop-call-button", As this is being added to the default placements, have you run this through tryserver yet (mochitest-bc) to see if this will break any of the customizableui tests?

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #10) > Comment on attachment 8426270 [details] [diff] [review] > Show the Loop button by default and update the tooltip and panel text. > > Review of attachment 8426270 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/components/customizableui/src/CustomizableUI.jsm > @@ +200,5 @@ > > "bookmarks-menu-button", > > "downloads-button", > > "home-button", > > +#ifdef MOZ_LOOP > > + "loop-call-button", > > As this is being added to the default placements, have you run this through > tryserver yet (mochitest-bc) to see if this will break any of the > customizableui tests? Yep, I'd already tried a subset of tests locally, but tried it in full today and it was all green: https://tbpl.mozilla.org/?tree=Try&rev=882dc6984a10 Re loop.properties, that's included in here due to comment 1 - the changes are being made together.

Comment on attachment 8426274 [details] Screen Shot of Panel & Tooltip Looks great!

Landed on our reviewed-code-only integration branch: https://github.com/adamroach/gecko-dev/commit/8a25f204e6e6f8da8ade0688732c9e5b5fd08836 Closing for tracking purposes, I'll comment again with urls as we land this in Mercurial

Please use meaningful string ids in the future and update them if changing string content.

Verified fixed through previous smoketesting.