1313429 - Add initial macOS touchbar support

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, enhancement, P2)

Description

[Blake Winton (:bwinton) (:☕️)]

Just announced.  Safari supports it.  It would be nice if we could get out ahead of this…

Comment 1

[Kev Needham [:kev]]

I'd also like it if I could put the add-ons status bar there.

Comment 2

[Blake Winton (:bwinton) (:☕️)]

I think a useful first version would only let us add a configurable toolbar in that space.  Later iterations could let us move or copy the urlbar there, add duplicate buttons to it, or move tabs there, or expose it to WebExtensions.  But I propose all that should happen _after_ we get a first version in the product.

Comment 3

[:Harald Kirschner :digitarald]

Throwing another one in here, devtools tabs & buttons; can be tested early on dev edition.

Comment 4

[Markus Stange [:mstange]]

With XCode 8.1 and a version of 10.12.1 that is linked to at the bottom of https://developer.apple.com/macos/touch-bar/ , you can put a touch bar on your screen to test what other apps are doing.

Safari by default shows Back/forward, a very wide button that looks like their empty URL bar (search icon + the text "Search or enter website name") which just focuses the URL bar when tapped, and a new tab button. When more than one tab is open, they show previews of tab contents (which don't update live or even on tab switch; maybe only once on pageload even). Through "View -> Customize Touch Bar..." you can add more buttons, e.g. "Add Bookmark" or "Favorites Bar".
When a video is playing, Safari lets you control playback and scrub through the video from the touch bar.
When a text field is focused, you get the usual word suggestions.
When focus is in the URL bar, you get icons of the URL bar dropdown items.

In Firefox, the bar is always empty. Text suggestions don't show up. Chrome has the same problem with text fields in websites.

Comment 5

[:Gijs (he/him)]

This is not a bug about Firefox's theme.

Comment 6

[Robert Strong (they/them - no direct email)]

(In reply to :Gijs Kruitbosch from comment #5)
> This is not a bug about Firefox's theme.
It is doubtful that this is a bug with the integration with the shell either. These are typically widget bugs but since I know nothing about how the touchbar is implemented leaving it here for now.

Comment 7

[Robert Strong (they/them - no direct email)]

I see that there is a bug for adding the touchbar api. If this doesn't have to do with setting as default and similar settings then please move this to a more appropriate component... perhaps Firefox -> General if there isn't a specific one.

Comment 8

[Blake Winton (:bwinton) (:☕️)]

(Add a link to some research on how Apple's built-in apps use the touchbar.)

Comment 10

[Nomis101]

In a few weeks I will get my MacBook Pro with Touch Bar, so I can test any patch here. For Macs without a Touch Bar, I found this application, which will simulate an on-screen Touch Bar. For Firefox/Thunderbird this is currently empty.
https://github.com/bikkelbroeders/TouchBarDemoApp

Comment 11

[Eric Shepherd [:sheppy]]

There's a lot of neat stuff we could do with the Touch Bar. I'd love to, for instance, have the dev tools toolbar always displayed there. I use them all the time but not quite all the time enough to want to lose that screen space.

But the idea I'm most excited about: We could use create a DOM window mapped to the Touch Bar's graphics context, so that you could then draw Web content into the Touch Bar from a WebExtension. Imagine an API that lets extensions add various types of controls or even a Canvas with its touch events to the Touch Bar, thereby making this an offscreen but still user-visible and interactable area for extensions to present information and interact with users.

Dev Tools could be presented there. Stats and performance data could be shown there in real time reflecting the current browser context. A "control strip" of extension buttons could be there for activating favorite extensions -- used just like the space in the standard tool bars at the top of the browser window, but on a global basis. So many ways this could be amazing.

Comment 12

[Erica Wright [:ewright]]

Attachment: Bug 1313429 - WIP, not looking for reviews. Add mac touchbar buttons for a few functions.

Review commit: https://reviewboard.mozilla.org/r/154584/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/154584/

Comment 13

[Nomis101]

FYI, the today released Chrome 60 Update added Touch Bar Support.


(In reply to Erica from comment #12)
> Created attachment 8883688 [details]
> Bug 1313429 - WIP, not looking for reviews. Add mac touchbar buttons for a
> few functions.

Thanks for the first patch. As I can see, this WIP is for Firefox and for unified builds only, right?

Comment 14

[Erica Wright [:ewright]]

Comment on attachment 8883688 [details]
Bug 1313429 - WIP, not looking for reviews. Add mac touchbar buttons for a few functions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154584/diff/1-2/

Comment 15

[Rhys Powell]

I've just messed around with Chrome's implementation of this some, and I think we can do a lot better. I really like the idea of extensions being able to provide Touch Bar items, as well as Eric's idea of using it to enhance the dev tools. Aside from that, I'd also add that I really like Safari's implementation and think that's worth looking to for inspiration, specifically the way it shows your bookmarks on the touch bar when you open a new tab.

Comment 16

[Stuart Philp :sphilp]

activity stream's top sites would be neat, or the ability to use "customize" to drag any ui element down there for quick access (similar to how OSX handles the control strip customization)

Comment 17

[(not currently active) Ted Mielczarek]

Attachment: Safari Touch Bar

TIL you can screenshot the touchbar with Cmd+Shift+6. This is what Safari displays there on my mac running 10.12.6.

Comment 18

[(not currently active) Ted Mielczarek]

Attachment: Chrome Touch Bar

...and this is what Chrome has there. I was doing something in Chrome and noticed that they have a reload button there, which is nice because apparently I have muscle memory for hitting F5 to reload, and this lets me do it without first having to press Fn.

Otherwise both browsers have back/forward, a URL bar shortcut, and something that looks like tab management.

Comment 19

[Patrick Albrecht]

Attachment: safari-open-tabs.png

Attached the screenshot of Safari in a new tab. The icons shown are the favorite bookmarks.
In Firefox that could be used either somehow for the activity stream or just to display some bookmarks. The most difficult part here is to find nice Icons to display them.

Comment 20

[Patrick Albrecht]

Attachment: safari-open-tabs.png

This is how Safari looks when you are in one of the open tabs. With touching a other tab you can switch between the tabs which is very nice. The button at the right end allows to open a new tab.

Comment 21

[Patrick Albrecht]

Attachment: safari-spellcheck.png

The last thing worth to mention is the spellcheck, it appears automatically if you type in something. It's technically a system wide function so maybe we could use it without redesign everything etc.

Comment 22

[Artem M]

It would be great if we could add "Save to Pocket" and "Enter Reader View" buttons there...

Comment 23

[John Gruen [:jgruen]]

Screenshots!

Comment 24

[Harry Twyford]

Attachment: Bug 1313429 - Implements basic Touch Bar functions.

Review commit: https://reviewboard.mozilla.org/r/251662/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/251662/

Comment 25

[Harry Twyford]

I’ve made some changes to Erica’s code, but hit a weird bug.

As for the changes, the code is functionally almost the same, but it handles how actions are performed a little differently. Originally, actions were stored as NS_STRINGs in an array, and then each Touch Bar button had an index into that array. Now, the identifiers of Touch Bar inputs are a string consisting of both their type and their action. For example, there could be a “com.mozilla.firefox.touchbar.button.View:FullScreen” or a “com.mozilla.firefox.touchbar.popover.cmd_find”. The idea here is that in the future if/when this is opened up to an API, XUL actions could be defined when the Touch Bar inputs are created. This also opens it up to different Touch Bar input types (popover, groups, video controls…) in the future. It’s not there yet, but one could see the cookie-cutter code in createTemplateItems being replaced with some kind of external (WebExtension) API.

This patch also adds a “Customize Touch Bar…” menu item, which is where the trouble comes in. Firefox crashes if the mouse enters the touch bar during the customization view, and I can’t figure out why. I’m looking for any advice or feedback on how we might fix this customization issue. Right now, the menu bar item to enter Customize mode is wired up to Apple’s toggleTouchBarCustomizationPalette: method in nsMenuBarX.mm.

To make diagnosing the issue a little easier, this patch never calls any of the code in TouchBar.mm or uses any of the new SVGs. It just uses a very basic Touch Bar implementation in nsCocoaWindow.mm that’s more or less straight out of an Apple tutorial. Once this customization issue is fixed, that code will be removed.

Comment 26

[Richard Marti (:Paenglab)]

Couldn't the (NSSet<NSTouchBarItem *> *)createTemplateItems {} and the icons move to /browser? Then Thunderbird could define his own set of buttons and icons.

Comment 27

[Harry Twyford]

Eventually, there may be some kind of front-end API in /browser so buttons can be defined more easily by other projects like Thunderbird or DevTools. This patch isn't done, but rather I just posted it to get through this customization bug. Part of the reason for the re-write of the initial patch was to make an API-like approach a bit easier in the future!

Comment 28

[Harry Twyford]

Comment on attachment 8986298 [details]
Bug 1313429 - Implements basic Touch Bar functions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251662/diff/1-2/

Comment 29

[Harry Twyford]

Attachment: touchbar_wip_customize_scrubber.patch

I’m submitting two different patches — one for review and one attached to this comment as a diff.

The patch for review is a basic proof-of-concept for the Touch Bar. It has a few buttons and contains telemetry. I’ve removed the users’ ability to customize the layout of the Touch Bar since I still haven’t been able to resolve the customization bug I mentioned above. Instead, different layouts can be toggled with the preference `ui.touchbar.layout`. Right now, there are only three layouts; as more buttons are added and as we test different layouts, I can certainly add more. 
Adding buttons and layouts is usually trivial, so long as the button executes one of these commands (https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/List_of_commands). Feel free to comment if you’d really like to see one of these commands in a revision to this particular patch.

The other patch is my current working commit. It has the customization code, and some WIP code to implement a control to scrub through open tabs, sort of like Safari’s Touch Bar tab scrubber (albeit without the flashy tiny page previews).  I’m including it in case it turns out someone else has to pick up work on customization; I’m not looking for a review on it (nor is it done) but of course anyone is free to comment.

I also haven’t yet written tests for either patch — I wasn’t sure which folder/framework they should go in. I’m still actively working on new features, but I thought I’d check in with these patches to highlight any major issues before I get too far ahead of myself.

Comment 30

[Stephen A Pohl]

Sorry about the delay here. Working on the review now.

Comment 31

[Mike de Boer [:mikedeboer]]

Hi Harry - fantastic start! I think you can eliminate all the hoops you seem to be jumping through to get contextually relevant touchbar layouts by deferring that logic to the frontend instead.
How about you create an XPCOM interface using this class, that passes in an array of items that it can load into the view in one go?

The frontend would become responsible for the layout and the backend (your code here) would remain as simple as possible, meaning that you wouldn't have to deal with localization and mapping actions to resources, etc.

```js
let touchBar = Cc["@mozilla.org/widget/mactouchbar;1"]
  .getService(Ci.nsIMacTouchBar);
touchBar.layout = [
  {
    title: "Back",
    image: "[e-mail]", /* Or an SVG we rasterized
                             on a black background? */
    type: "button", /* default is 'button', so can be
                       omitted. */
    command: "Back" /* upon item touch, dispatch a custom
                       event on the frontmost XUL window
                       with the command field as detail. */
  },
  {
    /* Title nil, will display image only. */
    image: "[e-mail]" /* Might be better to allow
                               only an ArrayBuffer here? */
    command: "Forward"
  },
  {
    type: "scrubber",
    /* Other scrubber-specific options may go here,
       but I hope that sensible defaults will make that
       unnecessary. */
    items: [
      { image: "Thumbnail1.png", command: "SelectTab1" },
      { image: "Thumbnail2.png", command: "SelectTab2" },
      { image: "Thumbnail3.png", command: "SelectTab3" }
    ]
  }
];

// Oh, hey! We've got new tabs added!
touchBar.setItemAt(2, {
  type: "scrubber",
  items: [
    { image: "Thumbnail1.png", command: "SelectTab1" },
    { image: "Thumbnail2.png", command: "SelectTab2" },
    { image: "Thumbnail3.png", command: "SelectTab3" },
    { image: "Thumbnail4.png", command: "SelectTab4" },
    { image: "Thumbnail5.png", command: "SelectTab5" }
  ]
});
```

On the frontend, in browser/modules/MacTouchBar.jsm, we'd have a singleton API entry-point that will be used to call these methods. So that way we can iteratively improve the usefulness of the touchbar integration.
Or phase it out whenever Apple decides that the next generation of MacBooks won't get a TouchBar anymore... :-P

This would also make it possible to clear the TouchBar layout when the Library window (or any other non-browser window) is active.
It would also allow you to let others contribute the scrubber stuff, a WebExtension API and other things I can't think of right now.

What do you think?

Comment 32

[Mike de Boer [:mikedeboer]]

(In reply to Harry Twyford [:harry] from comment #25)
> This patch also adds a “Customize Touch Bar…” menu item, which is where the
> trouble comes in. Firefox crashes if the mouse enters the touch bar during
> the customization view, and I can’t figure out why. I’m looking for any
> advice or feedback on how we might fix this customization issue. Right now,
> the menu bar item to enter Customize mode is wired up to Apple’s
> toggleTouchBarCustomizationPalette: method in nsMenuBarX.mm.

Let's workaround this issue by removing the menu item and not allowing this function for Gecko TouchBar layouts.

Comment 33

[Mike de Boer [:mikedeboer]]

To be clear, in IDL-speak, the `layout` attribute would be defined as `attribute nsIArray layout;`, just like we build Windows JumpLists (see WindowsJumpLists.jsm)

Comment 34

[Stephen A Pohl]

Comment on attachment 8986298 [details]
Bug 1313429 - Implements basic Touch Bar functions.

https://reviewboard.mozilla.org/r/251662/#review261116

This looks great and works really well in a local build!

I'm submitting this feedback as the first round of review. I will take a closer look at the cocoa specific things once this has been addressed.

1. The touchbar icons should go into a separate folder, since they aren't technically cursors. Also, they should either be provided by UX or signed off by UX.
2. For new methods and functions, attribute names should have an `a` prefix. For example: `aTouchbar` instead of `touchbar`.
3. Over 80 characters in several places.

::: commit-message-e3b45:1
(Diff revision 2)
> +Bug 1313429 - Implements basic Touch Bar functions. r?spohl,mstange

The commit message should be slightly more specific. For example:

Implement basic MacBook Touch Bar functionality and expose three different layouts via prefs.

::: modules/libpref/init/all.js:280
(Diff revision 2)
>  // Duration of timeout of incremental search in menus (ms).  0 means infinite.
>  pref("ui.menu.incremental_search.timeout", 1000);
>  // If true, all popups won't hide automatically on blur
>  pref("ui.popup.disable_autohide", false);
>  
> +#if defined(XP_MACOSX)

nit: s/if defined/ifdef/

::: toolkit/components/telemetry/Scalars.yaml:1750
(Diff revision 2)
>        - [e-mail]
>      release_channel_collection: opt-out
>      record_in_processes:
>        - 'content'
>  
> +# The following section contains Macbook Pro Touch Bar scalars

This will need a review from a telemetry peer. You should split everything telemetry-related into a separate patch to make it easier to review.

::: toolkit/locales/en-US/chrome/global/touchbar.properties:16
(Diff revision 2)
> +find = Find
> +newTab = New Tab
> +home = Home
> +addBookmark = Add Bookmark
> +openBookmarksSidebar = Open Bookmarks Sidebar
> +tabList = Tab List

tabList does not seem to be used at the moment. Is that right?

::: widget/cocoa/nsCocoaWindow.h:88
(Diff revision 2)
>  
>  - (NSRect)getAndResetNativeDirtyRect;
>  
>  - (void)setUseMenuStyle:(BOOL)aValue;
>  
> +- (NSTouchBar *)makeTouchBar;

nit: no space between NSTouchBar and *

::: widget/cocoa/nsCocoaWindow.mm:3168
(Diff revision 2)
>      [wrapper release];
>    }
>    mUseMenuStyle = aValue;
>  }
>  
> +- (NSTouchBar *) makeTouchBar

nit: no space between NSTouchBar and *, and between ) and makeTouchBar

::: widget/cocoa/nsTouchBar.h:20
(Diff revision 2)
> +* implementation only defines Touch Bar buttons for the main Firefox window. If modals and
> +* other windows were to have custom Touch Bar views, each window would have to be a
> +* NSTouchBarDelegate so they could define their own custom sets of buttons.
> +* NSTouchBarDelegate implements `makeTouchBar:` (in lieu of `init:`) and `makeItemForIdentifier`.
> +*/
> +@interface TouchBar : NSTouchBar<NSTouchBarDelegate>

Let's use nsTouchBar instead of TouchBar to make it clear that this is our internal implementation.

::: widget/cocoa/nsTouchBar.h:35
(Diff revision 2)
> +- (void)updateButton:(NSCustomTouchBarItem*)item
> +           withTitle:(NSString*)title
> +           withImage:(NSImage*)image;
> +
> +// Initializes buttons from a default set.
> +// Could be depreciated in the future if an API is introduced.

nit: deprecated, not depreciated.

::: widget/cocoa/nsTouchBar.mm:76
(Diff revision 2)
> +    NSTouchBarItemIdentifier OpenLocationButtonIdentifier =
> +      [CustomButtonIdentifier stringByAppendingPathExtension:OpenLocationIdentifier];
> +
> +    int layoutPref = [TouchBar getLayoutPref];
> +
> +    switch (layoutPref) {

nit: simply do |switch ([TouchBar getLayoutPref]) {| here and remove the local variable two lines earlier.

::: widget/cocoa/nsTouchBar.mm:100
(Diff revision 2)
> +
> +  return self;
> +}
> +
> +- (NSTouchBarItem*)touchBar:(NSTouchBar*)touchBar
> +     makeItemForIdentifier:(NSTouchBarItemIdentifier)identifier

nit: `:` should align with the previous line. Looks like you need one additional space before makeItemForIdentifier.

::: widget/cocoa/nsTouchBar.mm:135
(Diff revision 2)
> +}
> +
> +- (void)updateButtonFromDefaults:(NSCustomTouchBarItem*)item
> +                      withAction:(NSString*)action
> +{
> +  // if/elseif instead of switch/case since it's all string comparison

nit: no need for this comment

::: widget/cocoa/nsTouchBar.mm:141
(Diff revision 2)
> +  if ([action isEqualToString:BackIdentifier]) {
> +    [self updateButton:(NSCustomTouchBarItem*)item
> +             withTitle:[TouchBar getLocalizedTouchbarName:@"back"]
> +             withImage:[TouchBar cursorWithImageNamed:@"back@2x"]];
> +  }
> +  else if ([action isEqualToString:ForwardIdentifier]) {

please move all `else if` statements to the previous line, to match the following pattern:
if (...) {
  [...]
} else if (...) {
  [...]
} else {
  [...]
}

Style guide: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

::: widget/cocoa/nsTouchBar.mm:225
(Diff revision 2)
> +
> ++ (RefPtr<nsIDocument>)getXULDocument
> +{
> +    nsCOMPtr<nsIWindowMediator> wm = do_GetService(NS_WINDOWMEDIATOR_CID);
> +
> +    if (!wm)

Always uses curly braces, even for one line statements.

::: widget/cocoa/nsTouchBar.mm:241
(Diff revision 2)
> +}
> +
> ++ (void)dispatchSystemFunction:(nsAutoString)action
> +                    toDocument:(RefPtr<nsIDocument>)doc
> +{
> +  if (doc == nil)

Curly braces.

::: widget/cocoa/nsTouchBar.mm:270
(Diff revision 2)
> +  // A nil image will fail gracefully to a labelled button
> +
> +  return image;
> +}
> +
> ++ (NSString*)getLocalizedTouchbarName:(NSString*)lookupKey

Could you clarify where/how these localized names are exposed?

::: widget/cocoa/nsTouchBar.mm:281
(Diff revision 2)
> +  nsresult rv = sbs->CreateBundle("chrome://global/locale/touchbar.properties", getter_AddRefs(bundle));
> +  if (NS_SUCCEEDED(rv)) {
> +    nsAutoString localLabel;
> +      rv = bundle->GetStringFromName([lookupKey UTF8String], localLabel);
> +      if (NS_SUCCEEDED(rv)) {
> +      localizedName = [NSString stringWithCharacters:reinterpret_cast<const unichar*>(localLabel.get())

nit: should intend by two spaces.

Comment 35

[Harry Twyford]

Attachment: Bug 1313429 - Implements basic Touch Bar functionality via JS and exposes customization via a string preference.

Review commit: https://reviewboard.mozilla.org/r/256228/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/256228/

Comment 36

[Harry Twyford]

Attachment: Bug 1313429 - Store button presses on the Mac Touch Bar in a histogram.

Review commit: https://reviewboard.mozilla.org/r/256230/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/256230/

Comment 37

[Harry Twyford]

Mike, thanks for the review. I’m working on some IDL helper methods upstream, and can take a look at laying out all the buttons in the JS as well. While right now this patch only contains buttons that correspond to XUL commands, I’m concerned that future buttons with more advanced behaviour might not play well with a simple `command: string` set up. Do you know how I might make the command a JS callback or something along those lines, to allow for more custom behaviour?

Spohl, thanks as well for the review. I’ve made the recommended changes and spun out Telemetry into a separate commit.

Gijs and amin, I’ve r?’d you for a telemetry and ux review, respectively, if that’s alright?

Comment 38

[:Gijs (he/him)]

Comment on attachment 8991329 [details]
Bug 1313429 - Store button presses on the Mac Touch Bar in a histogram.

https://reviewboard.mozilla.org/r/256230/#review263230

Clearing review for now. I think someone like :chutten or :gfritzsche might be a better reviewer anyway. Sorry!

::: toolkit/components/telemetry/Scalars.yaml:1929
(Diff revision 1)
> +touchbar:
> +  enabled:
> +    bug_numbers:
> +      - 1313429
> +    description: >
> +      If the user has an enabled Touch Bar.

This is a little confusing, you don't record `false` to this when the user doesn't have a touch bar... Is there somewhere else we could put the telemetry to also explicitly record `false` (at least on macOS) ? That'd make it easier to do analysis after the fact.

Somewhat orthogonally, I think telemetry has some pretty specific system data (specifically, the environment JSON blob records the specific apple model id (e.g. MacBookPro14,1) for macs). Do we already have the data that way? Then we could avoid collecting it this way...

Comment 39

[Mike de Boer [:mikedeboer]]

(In reply to Harry Twyford [:harry] from comment #37)
> Mike, thanks for the review. I’m working on some IDL helper methods
> upstream, and can take a look at laying out all the buttons in the JS as
> well. While right now this patch only contains buttons that correspond to
> XUL commands, I’m concerned that future buttons with more advanced behaviour
> might not play well with a simple `command: string` set up. Do you know how
> I might make the command a JS callback or something along those lines, to
> allow for more custom behaviour?

In comment 31 I mentioned the following (somewhere in the code sample):

"Upon item touch, dispatch a custom event on the frontmost XUL window with the 'command' field as detail."

In other words: you won't need to worry about invoking a command at all, the frontend will be responsible with handling the event and either 1) invoke a command itself or 2) forward to the appropriate WebExtension.

If it proves to be (too) difficult to do event dispatching on a window from there, you can use the observer service instead. But as a last resort, ideally ;-)

Comment 40

[Amin Al Hazwani [:amin] (Firefox UX)]

Comment on attachment 8991328 [details]
Bug 1313429 - Implements basic Touch Bar functionality via JS and exposes customization via a string preference.

https://reviewboard.mozilla.org/r/256228/#review263390

Hey Harry, thanks for sharing this! I checked the icons and unfortunately they are not pixel snapped. For example if you check `[e-mail]` you can see that vertical strokes are blurred while they should be aligned to the pixel grid. I am going to export those icons for you and upload them to the bug. 

Some of the icons like `[e-mail]` exceed the ideal icons size of 36x36pt as recommended per apple guidelines https://developer.apple.com/design/human-interface-guidelines/macos/touch-bar/touch-bar-icons-and-images/#custom-icon-size. 

One last note, if we include the sidebar we should have two icons: one if users have the sidebar on the right and one if users have the sidebar on the right. I am going to attach both icons to the bug as well.

Comment 41

[Amin Al Hazwani [:amin] (Firefox UX)]

With the help of Johann we also trigger a build but it failed. Here's the link https://treeherder.mozilla.org/#/jobs?repo=try&revision=b49c8aca8b02f1e6f7e11705c17c83b5d930b0f8&selectedJob=187825173

Comment 42

[Amin Al Hazwani [:amin] (Firefox UX)]

I have the icons ready in Sketch, before exporting them, could you let me know what is the icons size that you're using in code (including bounding box). Is it 60x60pt as per Apple guidelines? Or do you draw the bounding box around the 36x36pt icon?

Comment 43

[Harry Twyford]

Thanks everyone, for taking a look! Amin, I supply 36x36px (or rather, 18x18pt @2x) icons, and Apple's code handles the bounding box.

Comment 44

[Amin Al Hazwani [:amin] (Firefox UX)]

Attachment: firefox-macos-touch-bar-icons.zip

Attaching the requested icons!

Comment 45

[Stephen A Pohl]

Comment on attachment 8991328 [details]
Bug 1313429 - Implements basic Touch Bar functionality via JS and exposes customization via a string preference.

Clearing review request for now per discussion via irc.

Comment 46

[Markus Stange [:mstange]]

Comment on attachment 8986298 [details]
Bug 1313429 - Implements basic Touch Bar functions.

https://reviewboard.mozilla.org/r/251662/#review264508

What's the status of this patch? Would you still like me to review it or are there changes coming?

Comment 47

[Harry Twyford]

Comment on attachment 8991328 [details]
Bug 1313429 - Implements basic Touch Bar functionality via JS and exposes customization via a string preference.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256228/diff/1-2/

Comment 48

[Harry Twyford]

Comment on attachment 8991329 [details]
Bug 1313429 - Store button presses on the Mac Touch Bar in a histogram.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256230/diff/1-2/

Comment 49

[Harry Twyford]

On Mike’s suggestion, this new patch makes use of XPIDL to create a JS API for defining Touch Bar inputs. If I’m not mistaken, the TouchBarHelper class has to be registered in a `.manifest` file, so it has to go in `/browser/components`. I couldn’t find an appropriate place to put the code without making a whole new `/browser/components/touchbar` folder, so `nsBrowserGlue.js` it was.

Just a couple more notes: 

* the `image`  field in the JS is the filename of an image that’s already defined in `widget/cocoa/resources/touchbar_icons`. I know this isn’t ideal, but I was looking to get a first patch out for review ASAP (the end of my internship approaches...) and so I settled on this. If this is a total no-go, I can write the code to read the images from the front-end based on `nsMenuItemIconX`

* The sidebar icon doesn’t toggle left/right yet. Seems like the browser accomplishes this with a XUL observer https://searchfox.org/mozilla-central/source/browser/components/customizableui/CustomizableWidgets.jsm#202 (?), but I wasn’t sure how to port this well to my code.

spohl, mikedeboer,  amin, and gfritzsche I’ve tagged you all for Cocoa/JS/UX/Telemetry reviews, respectively, if you have the time. mstange, please take a look if you’d like to!

Comment 50

[Amin Al Hazwani [:amin] (Firefox UX)]

Comment on attachment 8991328 [details]
Bug 1313429 - Implements basic Touch Bar functionality via JS and exposes customization via a string preference.

https://reviewboard.mozilla.org/r/256228/#review265124

Unfortunately my computer decided to abandom me and I am now working from a loaner without a Touch Bar, I also have problems downloading the raw file from Mercurial, if I click "raw" in this page https://reviewboard-hg.mozilla.org/gecko/file/964192a056c5/widget/cocoa/resources/touchbar_icons/back.pdf nothing happens. Could you add Stephen for UX review?

Comment 51

[Patrick Albrecht]

At least for the MacBook without touch bar there's a solution (if you didn't know that already): https://github.com/sindresorhus/touch-bar-simulator

Comment 52

[Amin Al Hazwani [:amin] (Firefox UX)]

And of course is from Sindre. Thank you Patrick! 

> I also have problems downloading the raw file from Mercurial

I manage to resolve this.

Comment 53

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8991329 [details]
Bug 1313429 - Store button presses on the Mac Touch Bar in a histogram.

https://reviewboard.mozilla.org/r/256230/#review265138

::: widget/cocoa/nsTouchBar.mm:287
(Diff revision 2)
> +  CopyCocoaStringToXPCOMString(telemetryKey, aTelemetryKey);
> +  Telemetry::ScalarAdd(Telemetry::ScalarID::TOUCHBAR_BUTTON_PRESSED, aTelemetryKey, 1);

What kind of data will be in this key string, besides `"share"`?
Do we always know at build-time which button labels we have?

If yes, i would recommend using a categorical histogram instead:
https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/telemetry/collection/histograms.html#categorical

This allows you to add a number of pre-defined labels, then collect a count for each label.
The data shows up on the Measurement Dashboard in a convenient graph like this:
https://mzl.la/2uNPsth

... compared to keyed scalars, which don't show up as easy to read:
https://mzl.la/2zTBBY1

Comment 54

[Georg Fritzsche [:gfritzsche]]

For context, adding new Telemetry probes doesn't require review from a Telemetry peer - but i'm happy to review or give feedback on it!

After you got code review on your patches, every new Telemetry probe however *does need data review*. You can find the details on this here:
https://wiki.mozilla.org/Firefox/Data_Collection#Requesting_Data_Collection

Comment 55

[Mike de Boer [:mikedeboer]]

Attachment: Diff patch, implementing more flexible callbacks instead of fixating commands

Harry, I went ahead and implemented support for callbacks on top of your current patch. You can safely import it on top of your commit by downloading the patch, and running `hg import --no-commit ~/Downloads/touchbar-callbacks.patch`.

I'm asking feedback because even though I might find using callback in the dictionary nicer, you or others might not.

There are a number of review points I'd like to make about the frontend part of the patch, but before I do that, I'd like to first see Stephen to take a look at the Cocoa parts, because that's the primary building block.
Stephen, when would you be free to provide a first round of feedback on the implementation?

It'd be great for two things to be added after that, but they're possible in respective follow-ups:
1. Allow data: URIs to be passed in as images, because I believe Cocoa supports initializing an NSImage from a URI. It'd be nice if they'd support SVGs that way, but I'm not sure. Haven't tried.
2. Implement custom scrubbers :-D
But they're scope-creep for this bug, so it's definitely for later.

I also noticed that my debug build mentions a shutdown leak of array items, so that's something you'll need to look at, I think. When a window closes and is cycle collected, the touchbar allocs need to be free, I reckon.

It was so cool to play around with your work applied, Harry. Thanks much!

Comment 56

[Stephen A Pohl]

(In reply to Mike de Boer [:mikedeboer] from comment #55)
> Stephen, when would you be free to provide a first round of feedback on the
> implementation?

I will be able to get back to this as soon as bug 1468053 is fixed.

Comment 57

[Harry Twyford]

Comment on attachment 8991328 [details]
Bug 1313429 - Implements basic Touch Bar functionality via JS and exposes customization via a string preference.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256228/diff/2-3/

Comment 58

[Harry Twyford]

Comment on attachment 8991329 [details]
Bug 1313429 - Store button presses on the Mac Touch Bar in a histogram.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256230/diff/2-3/

Comment 59

[Harry Twyford]

Mike, thanks so much for the feedback and diff! I definitely prefer the callback-style `command` field so I added it in. The new UUID field broke the reference that allowed buttons to update their state (like AddBookmark). I replaced it with a `key` field, which is just a duplicate of the `title` field, but it never gets fed into the localizer so it stays constant across locales.

Since spohl is tied up with 1468053 for the time being, I thought I’d slip in this one last patch before the weekend. It adds Mike’s callbacks and two more fields to TouchBarInput. The new fields are `color` (optional, hex color string, sets the background color,) and `context`, which is an optional callback that returns if the button should appear in the current context. It returns a string with a input’s name instead of a boolean, which is admittedly a bit weird, but I did this so that “inputs” that serve only to redirect to other inputs based on the context can be added. “OpenOrFocus” is like this; it and the related “Focus” buttons also make use of `color`!

I agree that the scope of this patch is ballooning (despite me just saying I added a bunch of features…) so I think `color` and `context` are it in terms of new features on this patch. Any additional features like scrubbers or Pocket/Screenshots buttons could be opened as separate bugs.

I will look at the memory leaks this weekend/early next week and address them.

Georg, thank you as well for your feedback. I’ve switched the Telemetry to a categorical histogram, and will request data review when appropriate.

Comment 60

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8991329 [details]
Bug 1313429 - Store button presses on the Mac Touch Bar in a histogram.

https://reviewboard.mozilla.org/r/256230/#review265634

This looks good to me from the Telemetry usage.
You should still with context on this project take a look over this.

Is there test coverage for this functionality?
It would be good to add test coverage for the Telemetry too, to avoid accidental breakage.

::: browser/components/nsBrowserGlue.js:2986
(Diff revision 3)
>                                    promptMessage, null, mainAction,
>                                    null, options);
>    },
>  };
>  
> -function execCommand(aCommandName) {
> +const touchBarTelemetry = Services.telemetry.getHistogramById("TOUCHBAR_BUTTON_PRESSES");

Do you need to keep this around as a global?
It doesn't have any real benefits.

Comment 61

[Richard Marti (:Paenglab)]

The browser keys seems to be hard coded in widget. What would be the approach when Thunderbird wants its own keys/images? Can they be added in widget too or is it planned to move them from widget to browser and TB can then define its own in his directory?

Comment 62

[Stephen A Pohl]

(In reply to Harry Twyford [:harry] from comment #49)
> On Mike’s suggestion, this new patch makes use of XPIDL to create a JS API
> for defining Touch Bar inputs. If I’m not mistaken, the TouchBarHelper class
> has to be registered in a `.manifest` file, so it has to go in
> `/browser/components`. I couldn’t find an appropriate place to put the code
> without making a whole new `/browser/components/touchbar` folder, so
> `nsBrowserGlue.js` it was.

The separate folder might be preferable here in order to keep things isolated from other unrelated code.

Comment 63

[Stephen A Pohl]

Comment on attachment 8991328 [details]
Bug 1313429 - Implements basic Touch Bar functionality via JS and exposes customization via a string preference.

https://reviewboard.mozilla.org/r/256228/#review265734

This currently fails to build on try due to errors in widget/cocoa code (see https://treeherder.mozilla.org/#/jobs?repo=try&revision=b49c8aca8b02f1e6f7e11705c17c83b5d930b0f8&selectedJob=187825173). Could you investigate and update the patch before I review? Thanks

::: browser/base/content/test/static/browser_all_files_referenced.js:46
(Diff revision 3)
>  ];
>  
>  // These are not part of the omni.ja file, so we find them only when running
>  // the test on a non-packaged build.
>  if (AppConstants.platform == "macosx")
> -  gExceptionPaths.push("resource://gre/res/cursors/");
> +  gExceptionPaths.push("resource://gre/res/cursors/").push("resource://gre/res/touchbar");

drive-by: let's break this into two lines. great opportunity to add curly braces too.

Comment 64

[Amin Al Hazwani [:amin] (Firefox UX)]

Comment on attachment 8991328 [details]
Bug 1313429 - Implements basic Touch Bar functionality via JS and exposes customization via a string preference.

https://reviewboard.mozilla.org/r/256228/#review266056

Two minor things to take into account: 
- Share menu: There is a second "Messagges" item at the end of the share list, will post screenshot
- Afaik we don't use "&" in strings, see "Erase & Close" during PB, will post screenshot

Comment 65

[Amin Al Hazwani [:amin] (Firefox UX)]

Attachment: Touch Bar Shot 2018-07-23 at 6.31.37 PM.png

Double "Messages" item in the share menu

Comment 66

[Amin Al Hazwani [:amin] (Firefox UX)]

Attachment: Touch Bar Shot 2018-07-23 at 6.42.52 PM.png

"&" in the "Erase & Close" button

Comment 67

[Harry Twyford]

Comment on attachment 8991328 [details]
Bug 1313429 - Implements basic Touch Bar functionality via JS and exposes customization via a string preference.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256228/diff/3-4/

Comment 68

[Harry Twyford]

Comment on attachment 8991329 [details]
Bug 1313429 - Store button presses on the Mac Touch Bar in a histogram.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256230/diff/3-4/

Comment 69

[Stephen A Pohl]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=34762124e7b7c7556863f8c90c96e13a10fe91c9

Comment 70

[Harry Twyford]

Comment on attachment 8991328 [details]
Bug 1313429 - Implements basic Touch Bar functionality via JS and exposes customization via a string preference.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256228/diff/4-5/

Comment 71

[Harry Twyford]

Comment on attachment 8991329 [details]
Bug 1313429 - Store button presses on the Mac Touch Bar in a histogram.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256230/diff/4-5/

Comment 72

[Harry Twyford]

Moving around the __attribute__(weak_import) lines appears to allow this new rev (rev 5) build on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ae0ebf3952107497ba6cad9f2db1acb934a5bc6

The same try run shows that there are failed tests regarding browser_all_files_referenced.js not including the Touch Bar localization file on Windows and Linux -- can anyone offer advice on how to exclude it entirely from those builds? It's only referenced in MacTouchBar.js, which is only included on Cocoa builds, so I'm not sure what to do on that one. I've added it to the list in browser_all_files_referenced.js for plaforms: ["macosx"].

Also between revs 4 and 5 of the patch, I think I've closed a bunch of the low-hanging memory leak issues. Try shows there are still a number of leaks -- any advice on where to look to close those?

Comment 73

[Harry Twyford]

Comment on attachment 8991328 [details]
Bug 1313429 - Implements basic Touch Bar functionality via JS and exposes customization via a string preference.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256228/diff/5-6/

Comment 74

[Harry Twyford]

Comment on attachment 8991329 [details]
Bug 1313429 - Store button presses on the Mac Touch Bar in a histogram.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256230/diff/5-6/

Comment 75

[Harry Twyford]

Thanks to some IRC help from spohl, this most recent patch builds on try and includes all the necessary Touch Bar files in the builds. I also fixed some small issues I've noticed and added some preliminary tests.
Here's a try run https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d625dd0d905233d951bdc26db688a3f77a9343e although somewhat embarrassingly that version is a little downstream from the patch I just posted so I think I'll fail my own then-unfinished tests once it gets unqueued... Regardless, it builds. Also my own tests pass now :)

My internship ends tomorrow (!) and then I'm vacationing for a bit. I'll try to address any feedback if any comes before tomorrow afternoon EST. Otherwise, I'll pick this back up as a contributor in mid-August to see it through reviews. Of course, if we want to get it out before then, anyone is free to pick up the work!

Thanks everyone for your help!

Comment 76

[Stephen A Pohl]

(In reply to Harry Twyford [:harry] from comment #75)
> Thanks to some IRC help from spohl, this most recent patch builds on try and
> includes all the necessary Touch Bar files in the builds. I also fixed some
> small issues I've noticed and added some preliminary tests.
> Here's a try run
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=6d625dd0d905233d951bdc26db688a3f77a9343e although
> somewhat embarrassingly that version is a little downstream from the patch I
> just posted so I think I'll fail my own then-unfinished tests once it gets
> unqueued... Regardless, it builds. Also my own tests pass now :)

Thank you for all the work here!

It looks like we're still leaking a ton of windows during the tests on try. We should get these tests to pass as I won't be able to give an r+ on the review before try is green. I should be able to look into this if we can't sort things out before your internship ends.

Comment 77

[Stephen A Pohl]

(In reply to Harry Twyford [:harry] from comment #72)
> The same try run shows that there are failed tests regarding
> browser_all_files_referenced.js not including the Touch Bar localization
> file on Windows and Linux -- can anyone offer advice on how to exclude it
> entirely from those builds? It's only referenced in MacTouchBar.js, which is
> only included on Cocoa builds, so I'm not sure what to do on that one. I've
> added it to the list in browser_all_files_referenced.js for plaforms:
> ["macosx"].

It looks like the current way to handle this is to open a bug similar to bug 1348526, reference the platforms where this file should not be included and then *exclude* it for those platforms in browser_all_files_referenced.js. Your fix did just the opposite in the platforms section. ;-) After filing the necessary bug, this should start to pass if you add the exclusion at the bottom of the whitelist in browser_all_files_referenced.js as follows:

// Bug XXXXXXX
{file: "chrome://global/locale/touchbar.properties", platforms: ["linux", "win"]},

Comment 78

[:Gijs (he/him)]

(In reply to Stephen A Pohl [:spohl] from comment #77)
> (In reply to Harry Twyford [:harry] from comment #72)
> > The same try run shows that there are failed tests regarding
> > browser_all_files_referenced.js not including the Touch Bar localization
> > file on Windows and Linux -- can anyone offer advice on how to exclude it
> > entirely from those builds? It's only referenced in MacTouchBar.js, which is
> > only included on Cocoa builds, so I'm not sure what to do on that one. I've
> > added it to the list in browser_all_files_referenced.js for plaforms:
> > ["macosx"].
> 
> It looks like the current way to handle this is to open a bug similar to bug
> 1348526, reference the platforms where this file should not be included and
> then *exclude* it for those platforms in browser_all_files_referenced.js.
> Your fix did just the opposite in the platforms section. ;-) After filing
> the necessary bug, this should start to pass if you add the exclusion at the
> bottom of the whitelist in browser_all_files_referenced.js as follows:
> 
> // Bug XXXXXXX
> {file: "chrome://global/locale/touchbar.properties", platforms: ["linux",
> "win"]},

You could just avoid shipping the unneeded locale file on Linux/Windows by using

#ifdef XP_MACOSX
// include file
#endif

in the locales/jar.mn file, right? Then the test will be happy and we won't be shipping unnecessary stuff to Windows/Linux users...

Comment 79

[Stephen A Pohl]

(In reply to :Gijs (he/him) from comment #78)
> You could just avoid shipping the unneeded locale file on Linux/Windows by
> using
> 
> #ifdef XP_MACOSX
> // include file
> #endif
> 
> in the locales/jar.mn file, right? Then the test will be happy and we won't
> be shipping unnecessary stuff to Windows/Linux users...

According to a comment in browser_all_files_referenced.js (which landed as part of bug 1349005), this doesn't seem to be possible:

// The l10n build system can't package string files only for some platforms.

If things have changed since then, we might be able to clean things up considerably by going through all the other files that aren't referenced on some platforms.

Comment 80

[:Gijs (he/him)]

(In reply to Stephen A Pohl [:spohl] from comment #79)
> (In reply to :Gijs (he/him) from comment #78)
> > You could just avoid shipping the unneeded locale file on Linux/Windows by
> > using
> > 
> > #ifdef XP_MACOSX
> > // include file
> > #endif
> > 
> > in the locales/jar.mn file, right? Then the test will be happy and we won't
> > be shipping unnecessary stuff to Windows/Linux users...
> 
> According to a comment in browser_all_files_referenced.js (which landed as
> part of bug 1349005), this doesn't seem to be possible:
> 
> // The l10n build system can't package string files only for some platforms.
> 
> If things have changed since then, we might be able to clean things up
> considerably by going through all the other files that aren't referenced on
> some platforms.

Ah, right, I had not seen that. There's checks for MOZ_FENNEC which made me think this should be OK, but I think certainly the way we ship lang-packs this might not work at the moment (and we do ship different ones for mobile v. desktop, but not for mac v. linux v. windows), so let's go with your suggestion then. Sorry for the noise!

Comment 81

[Stephen A Pohl]

Comment on attachment 8991328 [details]
Bug 1313429 - Implements basic Touch Bar functionality via JS and exposes customization via a string preference.

Harry's latest patch still shows some test failures on try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=757af8061a562a2fd7aac0afe6232e677de2f7c7

I will look at these and post a new patch once the issues are resolved. Clearing review requests for now.

Comment 82

[Stephen A Pohl]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=39cf4ce50f1ec2781a8bf08ccc30b61384aacfff

Comment 83

[Stephen A Pohl]

Attachment: Bug 1313429 - Implements basic Touch Bar functionality via JS and exposes customization via a string preference.

This is Harry's latest patch that I've updated slightly to pass all tests on try. I will send this to try one more time to confirm and will review the cocoa parts next.

Comment 84

[Stephen A Pohl]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7d779fbc6b908250bb3f2808e3ec9cf3e392aaa

Comment 85

[Stephen A Pohl]

Comment on attachment 8996396 [details] [diff] [review]
Bug 1313429 - Implements basic Touch Bar functionality via JS and exposes customization via a string preference.

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

A few comments and general things that need to be addressed:
1. All new files should just have the following comment at the top of the file:

  /* This Source Code Form is subject to the terms of the Mozilla Public
   * License, v. 2.0. If a copy of the MPL was not distributed with this
   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

2. This patch needs a review from a build peer.
3. The new IDLs probably need a dedicated review, unless :mikedeboer feels comfortable r+-ing these changes.
4. I will need to take another look at this patch once the comments below have been addressed, hence r- for now.
5. This review only covers the widget/cocoa parts.

::: widget/cocoa/nsTouchBar.h
@@ +8,5 @@
> +#define nsTouchBar_h_
> +
> +#import <Cocoa/Cocoa.h>
> +
> +#include "nsITouchBarHelper.h"

nit: order alphabetically

@@ +17,5 @@
> +#if !defined(MAC_OS_X_VERSION_10_12) || \
> +    MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_12
> +@interface NSButton(NewConstructors)
> +@property (nonatomic) BOOL imageHugsTitle;
> ++ (NSButton*)buttonWithTitle:(NSString *)title

nit: no space between NSString and *

@@ +37,5 @@
> +__attribute__((weak_import))
> +@interface NSTouchBarItem : NSObject
> +- (instancetype)initWithIdentifier:(NSTouchBarItemIdentifier)aIdentifier;
> +@end
> + 

nit: trailing white space

@@ +46,5 @@
> +@end
> +
> +@protocol NSTouchBarDelegate
> +@end
> + 

nit: trailing white space

@@ +78,5 @@
> +@property (strong) NSImage* image;
> +@property (strong) NSString* type;
> +@property (assign) nsCOMPtr<nsITouchBarInputCallback> callback;
> +@property (strong) NSColor* color;
> +@property (nonatomic) BOOL disabled;

This can be rewritten as:

@interface TouchBarInput : NSObject {
  NSString* key;
  NSString* title;
  NSImage* image;
  NSString* type;
  nsCOMPtr<nsITouchBarInputCallback> callback;
  NSColor* color;
  BOOL disabled;
}

@@ +80,5 @@
> +@property (assign) nsCOMPtr<nsITouchBarInputCallback> callback;
> +@property (strong) NSColor* color;
> +@property (nonatomic) BOOL disabled;
> +
> +- (id)initWithKey:(NSString*)aKey

Only the first component in the signature should contain "with". Every subsequent component should simply be named "title", "image" etc.

@@ +94,5 @@
> +@end
> +
> +/**
> + * Our TouchBar is its own delegate. This is adequate for our purposes,
> + * since the currentimplementation only defines Touch Bar buttons for the

nit: s/currentimplementation/current implementation/

@@ +107,5 @@
> + * Contains TouchBarInput representations of the inputs currently in
> + * the Touch Bar. Populated in `init` and updated by nsITouchBarUpdater.
> + */
> +@property (strong) NSMutableDictionary
> +  <NSTouchBarItemIdentifier, TouchBarInput*>* mappedLayoutItems;

This can be rewritten as:

@interface nsTouchBar : NSTouchBar<NSTouchBarDelegate,
                                   NSSharingServicePickerTouchBarItemDelegate,
                                   NSSharingServiceDelegate> {
  /**
   * Contains TouchBarInput representations of the inputs currently in
   * the Touch Bar. Populated in `init` and updated by nsITouchBarUpdater.
   */
  NSMutableDictionary
    <NSTouchBarItemIdentifier, TouchBarInput*>* mappedLayoutItems;
}

@@ +140,5 @@
> +                      forIdentifier:(NSTouchBarItemIdentifier)aIdentifier;
> +- (NSTouchBarItem*)makeShareScrubberForIdentifier:
> +                        (NSTouchBarItemIdentifier)aIdentifier;
> +
> +// Redirects button actions to the appropriate handler and handles telemetry.

nit: use /** [...] */ for method descriptions.

@@ +143,5 @@
> +
> +// Redirects button actions to the appropriate handler and handles telemetry.
> +- (void)touchBarAction:(id)aSender;
> +
> +- (void)dealloc;

nit: move this to just below - init

::: widget/cocoa/nsTouchBar.mm
@@ +6,5 @@
> +
> +#include "nsTouchBar.h"
> +
> +#include "nsDirectoryServiceDefs.h"
> +#include "nsIArray.h"

nit: order in alphabetical order

@@ +26,5 @@
> +static NSTouchBarItemIdentifier ShareScrubberIdentifier =
> +  [ScrubberIdentifier stringByAppendingPathExtension:@"Share"];
> +
> +// Used to tie action strings to buttons
> +static char identifierAssociationKey;

all static variables should have an `s`-prefix

@@ +28,5 @@
> +
> +// Used to tie action strings to buttons
> +static char identifierAssociationKey;
> +
> +nsCOMPtr<nsITouchBarHelper> touchBarHelper;

This should be a member of nsTouchBar.

@@ +29,5 @@
> +// Used to tie action strings to buttons
> +static char identifierAssociationKey;
> +
> +nsCOMPtr<nsITouchBarHelper> touchBarHelper;
> +nsCOMPtr<nsIArray> layoutItems;

This should be a local variable.

@@ +53,5 @@
> +    NSMutableArray<NSTouchBarItemIdentifier>* mutableDefaultItemIdentifiers =
> +      [NSMutableArray array];
> +    uint32_t itemCount = 0;
> +    layoutItems->GetLength(&itemCount);
> +    for (uint32_t j = 0; j < itemCount; ++j) {

nit: the name of the counter variable is typically `i`, unless we're dealing with nested for loops, `i` is already taken and we need a second counter.

@@ +59,5 @@
> +      if (!input) {
> +        continue;
> +      }
> +
> +      TouchBarInput* convertedInput =

TouchBarInput objects are not currently being released.

@@ +62,5 @@
> +
> +      TouchBarInput* convertedInput =
> +        [[TouchBarInput alloc] initWithXPCOM:input];
> +
> +      input = nil;

no need to set input to nil here.

@@ +64,5 @@
> +        [[TouchBarInput alloc] initWithXPCOM:input];
> +
> +      input = nil;
> +
> +      NSTouchBarItemIdentifier QueriedItemIdentifier =

nit: s/QueriedItemIdentifier/queriedItemIdentifier/

@@ +66,5 @@
> +      input = nil;
> +
> +      NSTouchBarItemIdentifier QueriedItemIdentifier =
> +        [nsTouchBar getIdentifierFromInput:convertedInput];
> +      [mutableDefaultItemIdentifiers addObject: QueriedItemIdentifier];

nit: no space between addObject: and queriedItemIdentifier.

@@ +69,5 @@
> +        [nsTouchBar getIdentifierFromInput:convertedInput];
> +      [mutableDefaultItemIdentifiers addObject: QueriedItemIdentifier];
> +
> +      // Add new input to dictionary for lookup of properties in delegate
> +      [self.mappedLayoutItems setObject:convertedInput

You may want to use subscripting to set and retrieve NSMutableDictionary values since it is less wordy. See https://developer.apple.com/documentation/foundation/nsmutabledictionary?language=objc.

@@ +73,5 @@
> +      [self.mappedLayoutItems setObject:convertedInput
> +                                 forKey:QueriedItemIdentifier];
> +    }
> +
> +    layoutItems = nil;

Once `layoutItems` is a local variable, there is no need to set it to nil here.

@@ +75,5 @@
> +    }
> +
> +    layoutItems = nil;
> +
> +    self.defaultItemIdentifiers = [mutableDefaultItemIdentifiers copy];

`self.defaultItemIdentifiers` is never released. Can you use NSDictionary's `allKeys` on `self.mappedLayoutItems` instead and do away with the local NSMutableArray completely? This would also avoid the need to manually release the array.

@@ +84,5 @@
> +
> +- (NSTouchBarItem*)touchBar:(NSTouchBar*)aTouchBar
> +      makeItemForIdentifier:(NSTouchBarItemIdentifier)aIdentifier
> +{
> +if ([aIdentifier hasPrefix:CustomButtonIdentifier]) {

nit: missing 2 space indent.

@@ +93,5 @@
> +                                     initWithIdentifier:aIdentifier];
> +    item.view = button;
> +
> +    return [self updateButton:item forIdentifier:aIdentifier];
> +  } else if ([aIdentifier hasPrefix:CustomMainButtonIdentifier]) {

This case can be folded into the preceding `if` case as there is no functional difference in the way it is handled.

@@ +120,5 @@
> +  if (!identifier) {
> +    return;
> +  }
> +
> +  [self.mappedLayoutItems setObject:aInput forKey:identifier];

You will have to release the current object, if there is one, before replacing it.

@@ +135,5 @@
> +             forIdentifier:identifier];
> +  }
> +}
> +
> +- (NSTouchBarItem*)updateButton:(NSCustomTouchBarItem*)aButton

It would be better to pass in a NSButton* and alloc/init the NSCustomTouchBarItem here instead.

@@ +154,5 @@
> +    button.image = input.image;
> +    [button setImagePosition:NSImageOnly];
> +  }
> +
> +  // If the button was previously disabled, it might not get updated if it's

s/it's/it has/ to make this easier to read/understand.

@@ +155,5 @@
> +    [button setImagePosition:NSImageOnly];
> +  }
> +
> +  // If the button was previously disabled, it might not get updated if it's
> +  // lost its `disabled` field somehow. This force-resets it.

Is this a problem that you have observed? Do you know how a button can 'lose' its `disabled` field?

@@ +182,5 @@
> +  NSButton* button = (NSButton*)aMainButton.view;
> +  button.imageHugsTitle = YES;
> +  [button setImagePosition:NSImageLeft];
> +  [button.widthAnchor
> +    constraintGreaterThanOrEqualToConstant:252]

Where does this magic number come from? Can we declare it as a constant at the top of this file?

@@ +189,5 @@
> +                      forOrientation:NSLayoutConstraintOrientationHorizontal];
> +  return aMainButton;
> +}
> +
> +- (NSTouchBarItem*)makeShareScrubberForIdentifier:

When I click on the 'share' button for the first time, I get the following warning in the console:

2018-08-15 22:55:32.738 firefox[80254:21789226] warning: illegal subclass SHKRemoteView instantiating; client should use only NSRemoteView (
	0   ViewBridge                          0x00007fff69f56bff -[NSRemoteView _preSuperInit] + 195
	1   ViewBridge                          0x00007fff69f56f83 -[NSRemoteView initWithFrame:] + 25
	2   ShareKit                            0x00007fff66236aa5 -[SHKRemoteView initWithOptionsDictionary:] + 161
	3   ShareKit                            0x00007fff66215fbd __38-[SHKSharingService performWithItems:]_block_invoke_4 + 1347
	4   libdispatch.dylib                   0x00007fff6d29b5fa _dispatch_call_block_and_release + 12
	5   libdispatch.dylib                   0x00007fff6d293db8 _dispatch_client_callout + 8
	6   libdispatch.dylib                   0x00007fff6d29f395 _dispatch_main_queue_callback_4CF + 1148
	7   CoreFoundation                      0x00007fff454cac19 __CFRUNLOOP_IS_SERVICING_THE_MAIN_DISPATCH_QUEUE__ + 9
	8   CoreFoundation                      0x00007fff4548cdfa __CFRunLoopRun + 2586
	9   CoreFoundation                      0x00007fff4548c153 CFRunLoopRunSpecific + 483
	10  HIToolbox                           0x00007fff44776d96 RunCurrentEventLoopInMode + 286
	11  HIToolbox                           0x00007fff44776a0f ReceiveNextEventCommon + 366
	12  HIToolbox                           0x00007fff44776884 _BlockUntilNextEventMatchingListInModeWithFilter + 64
	13  AppKit                              0x00007fff42a27a73 _DPSNextEvent + 2085
	14  AppKit                              0x00007fff431bde34 -[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:] + 3044
	15  XUL                                 0x000000010c761e4c -[GeckoNSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 220
	16  AppKit                              0x00007fff42a1c885 -[NSApplication run] + 764
	17  XUL                                 0x000000010c762e27 _ZN10nsAppShell3RunEv + 247
	18  XUL                                 0x000000010d905514 _ZN12nsAppStartup3RunEv + 68
	19  XUL                                 0x000000010da12934 _ZN7XREMain11XRE_mainRunEv + 3508
	20  XUL                                 0x000000010da1327c _ZN7XREMain8XRE_mainEiPPcRKN7mozilla15BootstrapConfigE + 1164
	21  XUL                                 0x000000010da13911 _Z8XRE_mainiPPcRKN7mozilla15BootstrapConfigE + 193
	22  firefox                             0x0000000101a280ab main + 523
	23  libdyld.dylib                       0x00007fff6d2cd015 start + 1
	24  ???                                 0x0000000000000005 0x0 + 5
)

We may want to investigate what triggers this warning.

@@ +226,5 @@
> +    [item release];
> +    item = nil;
> +  }
> +
> +  [self.mappedLayoutItems removeAllObjects];

You have to individually release these objects before removing them from the dictionary.

@@ +232,5 @@
> +}
> +
> +#pragma mark - TouchBar Utilities
> +
> ++ (NSTouchBarItemIdentifier)getIdentifierFromInput:(TouchBarInput*)aInput

The native identifier (NSTouchBarItemIdentifier) should be a property of TouchBarInput. This will avoid the need for this utility function.

@@ +269,5 @@
> +  NS_ENSURE_SUCCESS(rv, nil);
> +
> +  pathToImage = [NSString stringWithUTF8String:(const char*)resPath.get()];
> +  pathToImage = [pathToImage stringByAppendingPathComponent:aImageName];
> +  NSImage* image = [[[NSImage alloc] 

nit: trailing whitespace

@@ +289,5 @@
> +  if (touchBarHelper) {
> +    nsresult rv = touchBarHelper->GetActiveUrl(url);
> +    nsresult rv2 = touchBarHelper->GetActiveTitle(title);
> +    if (!NS_FAILED(rv)) {
> +      urlToShare = [NSURL URLWithString:nsCocoaUtils::ToNSString(url)];

You may want to check for a valid URL, or at least for a non-empty string. If the page is blank and the user clicks the share button, the following is printed to the console:

2018-08-15 22:55:06.674 firefox[80254:21789226] NSURLs written to the pasteboard via NSPasteboardWriting must be absolute URLs.  NSURL '' is not an absolute URL.

@@ +312,5 @@
> +{
> +  // redundant services
> +  NSArray* excludedServices = @[
> +    @"com.apple.share.System.add-to-safari-reading-list",
> +    @"com.apple.share.Mail.compose",

Why is composing an email excluded?

::: widget/cocoa/nsTouchBarUpdater.mm
@@ +7,5 @@
> +#include "nsTouchBarUpdater.h"
> +#include "nsITouchBarInput.h"
> +#include "nsTouchBar.h"
> +
> +#include "nsIWebNavigation.h"

nit: order alphabetically

@@ +10,5 @@
> +
> +#include "nsIWebNavigation.h"
> +#include "nsIWidget.h"
> +#include "nsCocoaFeatures.h"
> +#include "nsIDocShellTreeItem.h"

trim the list of includes once we pass in a nsIBaseWindow* to nsTouchBarUpdater::UpdateTouchBarInput (see below).

@@ +28,5 @@
> +
> +NS_IMPL_ISUPPORTS(nsTouchBarUpdater, nsITouchBarUpdater);
> +
> +nsresult
> +GetBaseWindow(nsIDocShell* aWindow, BaseWindow** aCocoaWindow)

This function should be declared `static`. But read my comments in UpdateTouchbBarInput below. I believe this function will become obsolete.

@@ +30,5 @@
> +
> +nsresult
> +GetBaseWindow(nsIDocShell* aWindow, BaseWindow** aCocoaWindow)
> +{
> +  NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT;

There is no need for this try/abort block, since we're not calling out to native code.

@@ +58,5 @@
> +  NS_OBJC_END_TRY_ABORT_BLOCK_NSRESULT;
> +}
> +
> +NS_IMETHODIMP
> +nsTouchBarUpdater::UpdateTouchBarInput(nsIDocShell* aWindow,

You should be able to pass in a nsIBaseWindow* directly and avoid calling GetBaseWindow.

@@ +61,5 @@
> +NS_IMETHODIMP
> +nsTouchBarUpdater::UpdateTouchBarInput(nsIDocShell* aWindow,
> +                                       nsITouchBarInput* aInput)
> +{
> +  NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT;

We aren't calling out to native code here, so we shouldn't need the try/abort block.

@@ +67,5 @@
> +  BaseWindow* cocoaWin;
> +  nsresult rv = GetBaseWindow(aWindow, &cocoaWin);
> +  if (NS_FAILED(rv) || !cocoaWin) {
> +    return NS_ERROR_FAILURE;
> +  }

Once we pass in a nsIBaseWindow*, this section could simply be rewritten as follows:

  nsCOMPtr<nsIWidget> widget = nullptr;
  aWindow->GetMainWidget(getter_AddRefs(widget));
  if (!widget) {
    return NS_ERROR_FAILURE;
  }
  BaseWindow* cocoaWin = (BaseWindow*)widget->GetNativeData(NS_NATIVE_WINDOW);
  if (!cocoaWin) {
    return NS_ERROR_FAILURE;
  }

::: widget/nsITouchBarHelper.idl
@@ +3,5 @@
> + * This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "nsISupports.idl"

nit: order alphabetically

::: widget/nsITouchBarInput.idl
@@ +2,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "nsISupports.idl"

nit: order alphabetically

@@ +40,5 @@
> +     *   `button`: A standard button.
> +     *             If an image is available, only the image is displayed.
> +     *   `mainButton`: An extra-wide button. Displays both the image and title.
> +     *   `scrubber`: A Scrubber element. Not yet implemented, except in the
> +     *               exeception case of Apple's pre-built Share scrubber.

nit: s/except in the exeception case/except in the case/

::: widget/nsITouchBarUpdater.idl
@@ +3,5 @@
> + * This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "nsISupports.idl"

nit: order alphabetically

Comment 86

[Mike de Boer [:mikedeboer]]

Comment on attachment 8996396 [details] [diff] [review]
Bug 1313429 - Implements basic Touch Bar functionality via JS and exposes customization via a string preference.

I think it'd be good to continue with the following to help speed up the review cycles:

 - Split up the patch into multiple, smaller chunks in logical order. Each of the patches will become a commit.
   - For example: two commits for the backend, first would be the introduction of the new touchbar classes and the second would be the Gecko integration points (if that makes sense). Same for the frontend: one for the new files and the second to integrate it. Third frontend commit would be the new tests and the fourth would be the changes to existing tests. How does that sound? I'd also recommend a separate patch for the binary additions - even though I'd hope they can also be added as SVGs, instead, so we can review those as well.
   - This allows reviewers to grant r+'s much earlier, which is just so much nicer to do.
   - Having a large change divided into multiple chunks/ commits helps future contributors to learn the code in by-commit steps.
   - You can ask other reviewers to review the separate chunks, effectively distributing the review-/ workload, resulting in getting your code into a land-able state much quicker!
 - Try to address Stephen's review comments first - I think it's most important to get the 'backend' side of things into shape first. Yes, this is a classic order things, but still seems to work best in modern practice.

I hope you find this feedback useful! As always, please feel free to reach out any time for help, discussion or just to say hi. ;-)

Again, I think the primary goal at this point is to get your awesome contribution into a shape where we can get the process running much more smoothly.

Comment 87

[Harry Twyford]

No patch to present (yet!), but I just wanted to check in. I just got back from a trip on Sunday night, hence the silence on the thread. Thank you Stephen and Mike for the reviews! You both recommended some great changes. I've incorporated Stephen's changeset on my local build, and I will divide the patch into parts as per Mike's suggestion ASAP and repost. Mike, I agree its best to focus on getting the patch into a shape where it can ship. Thanks again!

Comment 88

[Harry Twyford]

Attachment: Bug 1313429 - Add Touch Bar functionality to Firefox

Add base nsTouchBar classes and nsCocoaWindow integration point.

Comment 89

[Harry Twyford]

The above is my multi-part patch, as per Mike's suggestion. Sorry it's so late; finding out MozReview has been sunset and then setting up Phabricator and getting familiar with it was a multi-day comedy of errors for me. This is my first time posting to Phabricator, so it's very possible I mishandled some aspect of posting this patch. Please let me know if I should post it to Phabricator in some other way. Fingers crossed I can figure out the new review/response workflow...

spohl, I incorporated almost all of your changes. Addressing the lengthy output when pressing the Share button may be outside the scope of this patch: the same output is printing even with the existing macOS share-sheet code. Also, I did not change the updateButton method to take an NSButton instead of a NSCustomTouchBarItem. Both the update and creation of new NSTouchBarItems call updateButton. The updateItem method uses the Cocoa `[nsTouchBar itemForIdentifier]` method to fetch the NSTouchBarItem, and then updateButton updates both the NSTouchBarItem and the NSButton it contains, so passing just the NSButton wouldn't work there. I can explain in more detail on IRC, if you would like. It's likely possible to rework the  NSTouchBarItem creation/update process if passing a NSTouchBarItem to updateButton is a no-go.

Comment 90

[Harry Twyford]

Two more quick things:
- I didn't tag anyone for review on the telemetry commit since it is the same as the one above, which was already r+'d by gfritzsche.
- spohl, I tagged you on the commit with the binary icon files. The icons themselves have already been r+'d by amin, so you are tagged simply to look at the code that adds the build rules for the new directory and files and so on.

Thanks everyone! :)

Comment 91

[Marcia Knous [:marcia]]

Hello Harry - It looks as if there are changes requested for https://phabricator.services.mozilla.com/D5496 - it appears as if that comment was made after you made Comment 90, so I thought I would let you know.

Comment 92

[Harry Twyford]

Thanks for the needinfo Marcia. I was swept up with school and fell behind on this. The nudge reminded me to finish this work!

I've posted a new revision. Mike, I addressed your review. Thank you! spohl, I made a small change to the init method in nsTouchBar to fix a bug where the buttons weren't in order. Could you please take a look? As I mentioned in comment 89, Phabricator is still a little foreign to me, so somebody please let me know if I've gone about posting the new patch the wrong way.

Comment 93

[Stephen A Pohl]

Hi Harry, just making sure that you saw the review feedback in Phabricator. Thanks for your continued work here!

Comment 94

[Harry Twyford]

Hi spohl, I saw the feedback. Thanks! The fix to dealloc was quick, but the changes recommended to switch the localization to Fluent have tripped me up. I'll push the dealloc changes now and make the Fluent changes a separate patch when they're done.

Comment 95

[Stephen A Pohl]

Hi Harry, have you had success with the switch to Fluent? Is there anything that you need to be able to move this along? Thanks!

Comment 96

[Harry Twyford]

Hi! Thanks for the nudge. I've made a few attempts at the switch to Fluent (including just now), but can't seem to manage resolving the async JS call to the Localization class in time for the GetTitle() call on the Obj-C side. 

I searched around for an example of Fluent being used like this -- where the Localization class is loaded in the front-end, but the localized text is ultimately used in the back-end, but couldn't find anything useful. This might just be a matter of some async JS tricks I'm not yet familiar with to get everything resolved in time, or maybe I'll have to resolve the Promise in the Obj-C code?

Can anyone point me to other code that does something along these lines? Alternatively, flod and Pike suggested I could put the .properties file somewhere that isn't exposed to localization for the time being, until either I or someone else can make the Fluent switch. flod, I've needinfo'd you. Could you please elaborate?

Sorry for the delays on getting this out the door!

Comment 97

[Francesco Lodolo [:flod]]

Redirecting to Pike, since I don't expect to be around much for the next 10 days.

Comment 98

[Mike de Boer [:mikedeboer]]

And perhaps Zibi has ideas for the implementation? (see comment 96 and Phabricator reviews).

Comment 99

[Stephen A Pohl]

(In reply to Harry Twyford [:harry] from comment #96)
> Sorry for the delays on getting this out the door!

No worries. Please let me know if you get stuck. Hopefully, Zibi and/or Pike can provide all the info you need.

Comment 100

[Axel Hecht [:Pike]]

I would make getTouchBarInput async, and set the title from the fluent string explicitly. And then remove the reference to the stringbundle in the title getter.

You'd need to reget the touchbarinput on intl:app-locales-changed, https://firefox-source-docs.mozilla.org/intl/locale.html#events

Comment 101

[Stephen A Pohl]

Hey Harry, would you mind posting your current patch with the Fluent changes? I'm happy to take a look. Thanks!

Comment 102

[Harry Twyford]

Posted. As I mentioned in the diff notes, this diff doesn't load anything in the Touch Bar. The issue is that the ObjC code calls `get layout()` in the JS which now returns an nsIArray of nsITouchBarInput Promises rather than nsITouchBarInputs themselves.

I've tried a couple of different ways of resolving the Promises or making different objects the Promise (such as the nsIArray itself or the nsITouchBarInput.title property) to get around a Promise being passed to the backend but that didn't work. At the end of the day, I think _some_ Promise will have to be resolved on the ObjC side. Thank you for taking a look!

Comment 103

[Tim Nguyen :ntim]

Harry, since you already have an updateTouchBarInput() function that can update a touch bar input at any point in time, maybe it's possible to await the promise on the front-end side, and then use updateTouchBarInput() (or some modified version of it) after the promise has been awaited. And initially, in get layout(), you could return dummy strings.

That wouldn't be a very elegant solution (because updateTouchBarInput is probably not meant to be used that way), but I think it could work, and from there you probably will be able to refactor a bit to make it a bit more elegant.

What do you think ?

Comment 104

[Harry Twyford]

Hi Tim, thanks for the nudge. That's a great idea! I hadn't thought of that. Working through it in my head I don't see there being issues with that approach, other than it being a bit inelegant, as you mentioned. I'll give an implementation a shot and report back here soon on if it works or not.

Comment 105

[Harry Twyford]

It worked -- thanks Tim! I posted a new revision on Phabricator. The change affects both the main JS and .mm files, so :mikedeboer and :spohl, could you please take another look?

Updating the Touch Bar inputs after their initial load leads to the buttons loading before their text labels. This only affects the "mainButton" type inputs (the wide ones, like the main focus URL Bar button in the middle). I've handled this by blanking out the entire input while the "load in" happens. On my machine, this takes about half a second. I've added some caching so there's no delay on opening new windows or when Firefox was running in the background. I'd appreciate feedback on if the "load in" effect is acceptable.

Comment 106

[Tim Nguyen :ntim]

Nice, excited to see this awesome work closer to landing! I've pushed this to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f8b680342bd3fb894d0b0dc77ef8164baff2669 (with the .properties file removed)

ni? mikedeboer and spohl for comment 105. Here's the interdiff since the last review in case it makes things easier: https://phabricator.services.mozilla.com/D5496?vs=34190&whitespace=ignore-all#toc

Comment 107

[Tim Nguyen :ntim]

New try push with the macOS build failure fixed (see my comment on Phabicator): https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b4a0c7dccf2d96f002a5c4ea365bdeb6baaa990

Comment 108

[Stephen A Pohl]

(In reply to Tim Nguyen :ntim from comment #106)

Nice, excited to see this awesome work closer to landing! I've pushed this to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f8b680342bd3fb894d0b0dc77ef8164baff2669 (with the .properties file removed)

ni? mikedeboer and spohl for comment 105. Here's the interdiff since the last review in case it makes things easier: https://phabricator.services.mozilla.com/D5496?vs=34190&whitespace=ignore-all#toc

Thanks, :ntim and :harry!

Comment 109

[Zibi Braniecki [:zbraniecki][:gandalf]]

Tim - if you cache the results of localization, can you also invalidate the cache (using intl:app-locales-changed event)?

Comment 110

[Tim Nguyen :ntim]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #109)

Tim - if you cache the results of localization, can you also invalidate the cache (using intl:app-locales-changed event)?

Harry already seems to be doing it in the observe function of MacTouchBar.js.

Comment 111

[Zibi Braniecki [:zbraniecki][:gandalf]]

Ahhh, you're right! Looks good to me!

Comment 112

[Tim Nguyen :ntim]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bda8c07f79e49a76acc235c10cf43078dbe23a73

Comment 113

[Tim Nguyen :ntim]

browser_touchbar_tests.js seems to crash (this is after rebasing the patch, and fixing up the input names in the test).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bda8c07f79e49a76acc235c10cf43078dbe23a73&selectedJob=222174460

Comment 114

[Tim Nguyen :ntim]

This looks relevant: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=222170933&repo=try&lineNumber=12871-12885

02:09:48 INFO - GECKO(1080) | 2019-01-16 02:09:48.125 firefox[1080:16262] -[ToolbarWindow touchBar]: unrecognized selector sent to instance 0x11c05de20
02:09:48 INFO - GECKO(1080) | Hit MOZ_CRASH(Unhandled exception) at /builds/worker/workspace/build/src/toolkit/crashreporter/nsExceptionHandler.cpp:1369
02:09:48 INFO - GECKO(1080) | #01: libc++abi.dylib + 0x260a1
02:09:48 INFO -
02:09:48 INFO - GECKO(1080) | #02: libc++abi.dylib + 0x25b30
02:09:48 INFO -
02:09:48 INFO - GECKO(1080) | #03: libobjc.A.dylib + 0xe898
02:09:48 INFO -
02:09:48 INFO - GECKO(1080) | #04: CoreFoundation + 0x1670ad
02:09:48 INFO -
02:09:48 INFO - GECKO(1080) | #05: CoreFoundation + 0xace24
02:09:48 INFO -
02:09:48 INFO - GECKO(1080) | #06: CoreFoundation + 0xac998
02:09:48 INFO -
02:11:19 INFO - GECKO(1080) | #07: nsTouchBarUpdater::UpdateTouchBarInput(nsIBaseWindow*, nsITouchBarInput*) [widget/cocoa/nsTouchBarUpdater.mm:41]
02:11:19 INFO -

Comment 115

[Tim Nguyen :ntim]

I can't actually reproduce the crash locally.

Here's a try push with the localization code commented out though: https://treeherder.mozilla.org/#/jobs?repo=try&revision=add09e506fb6cf8b21e3e4c4e569ce418c8aca3e

Just to bisect what's actually causing the crash.

Comment 116

[Harry Twyford]

Ditto to not being able to reproduce locally before and after rebasing on central. The [ToolbarWindow touchBar]: unrecognized selector sent to instance error might indicate that the call to updateTouchBarInput() after the l10n Promise is fulfilled is beating the initialization of the Touch Bar, so there's nothing to update on ToolbarWindow's instance of touchBar. I'll work on a fix for that to see if that's the issue.

Comment 117

[Stephen A Pohl]

unrecognized selector sent to instance means that the object does not respond to this method. Since our test machines run on hardware without touchbars, this isn't too surprising. We should add a respondsToSelector: check in nsTouchBarUpdater::UpdateTouchBarInput before calling updateItem.

Comment 118

[Tim Nguyen :ntim]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2588b9b1a91876d5d29efb63c375e5c865fbee38

Comment 119

[Sylvestre Ledru [:Sylvestre]]

Release Note Request (optional, but appreciated)
[Why is this notable]: New feature
[Affects Firefox for Android]: Mac specific
[Suggested wording]: Add OSX touchbar support
[Links (documentation, blog post, etc)]:

Comment 120

[Harry Twyford]

Attachment: request.txt

This patch collects telemetry, so here is the data review form.
(ed: whoops, it didn't render inline. I'll copy here for convenience:)

Request for data collection review form

1. What questions will you answer with this data?
   What buttons on the MacBook Pro Touch Bar are used most often?

2. Why does Mozilla need to answer these questions? Are there benefits for users? Do we need this information to address product or business requirements?
   Determine what inputs should be placed in the Touch Bar to maximize the benefit to users of Firefox's Touch Bar functionality.

3. What alternative methods did you consider to answer these questions? Why were they not sufficient?
   We could determine what users have Touch Bars then measure if their engagement with Firefox changes based on different Touch Bar layouts.
   This is very imprecise, seeing as overall engagement with the Touch Bar might be quite low, and Touch Bar layout is likely to have a low impact on overall Firefox engagement.

4. Can current instrumentation answer these questions?
   No. The Touch Bar is an entirely new component and we do not currently have any tools to measure it.

5. List all proposed measurements and indicate the category of data collection for each measurement, using the Firefox data collection categories on the Mozilla wiki.

<table>
<tr>
<td>Measurement Description</td>
<td>Data Collection Category</td>
<td>Tracking Bug #</td>
</tr>
<tr>
<td>Using a histogram, count the number of times a labeled button on the Touch Bar is pressed (e.g. "New Tab" pressed 45 times, "Share" pressed 3 times).</td>
<td>Interaction data</td>
<td>1313429</td>
</tr>
</table>

6. How long will this data be collected? Choose one of the following:
   I want to collect this data until the release of version 71 (about 8.5 months).

7. What populations will you measure?
   Any users with a MacBook Pro with Touch Bar, in all countries and locales.
   Measurement will start in Nightly and continue through Beta into Release as the Touch Bar feature matures.

8. If this data collection is default on, what is the opt-out mechanism for users?
   If users blank out the ui.touchbar.layout preference, no buttons will appear on the Touch Bar and thus there will be nothing to measure.

9. Please provide a general description of how you will analyze this data.
   We will look at which Touch Bar inputs are pressed most often in A/B experiments on the layout.
   For instance, is the Share button pressed more often when it is on the left side of the Touch Bar versus the right side?
   We will determine what buttons are the most or lesat popular based on the number of times they are pressed.

10. Where do you intend to share the results of your analysis?
    Internally, with those working on the Touch Bar feature. The UX team might also review the data if they request it.

Comment 121

[Chris H-C :chutten]

Comment on attachment 9037959 [details]
request.txt

Preliminary Notes:

You don't have to worry about it rendering or not rendering inline. I do my reviews in the Attachment Details view anyway :)

DATA COLLECTION REVIEW RESPONSE:

    Is there or will there be documentation that describes the schema for the ultimate data set available publicly, complete and accurate?

Yes. This collection is Telemetry so is documented in it definition file ([Histograms.json](https://hg.mozilla.org/mozilla-central/log/tip/toolkit/components/telemetry/Histograms.json)), in the [Probe Dictionary](https://telemetry.mozilla.org/probe-dictionary/), and on telemetry.mozilla.org's [Measurement Dashboard](https://telemetry.mozilla.org/new-pipeline/dist.html).

    Is there a control mechanism that allows the user to turn the data collection on and off? 

Yes. This collection is Telemetry so can be controlled in Firefox's Preferences.

    If the request is for permanent data collection, is there someone who will monitor the data over time?

N/A expires in Firefox 71.

    Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?

Category 2, Interaction

    Is the data collection request for default-on or default-off?

Default on for all channels. (The data collection only applies to users on Macbook Pros with touch bars)

    Does the instrumentation include the addition of any new identifiers (whether anonymous or otherwise; e.g., username, random IDs, etc. See the appendix for more details)?

No.

    Is the data collection covered by the existing Firefox privacy notice? 

Yes.

    Does there need to be a check-in in the future to determine whether to renew the data?

Yes. :mdeboer is responsible for removing or renewing this probe before it expires in Firefox 71.

---
Result: datareview+

Comment 122

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/099f631b9004
Add Touch Bar functionality to Firefox r=spohl,mikedeboer,flod

Comment 124

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/099f631b9004

Comment 125

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9509c720efe
Fix off-by-one issue from merge conflict between bug 1313429 and bug 1521460. CLOSED TREE

Comment 126

[Pascal Chevrel (relman team) -> :pascalc|PTO until Sept 2]

(In reply to Sylvestre Ledru [:sylvestre] from comment #119)

Release Note Request (optional, but appreciated)
[Why is this notable]: New feature
[Affects Firefox for Android]: Mac specific
[Suggested wording]: Add OSX touchbar support
[Links (documentation, blog post, etc)]:

Added to Nightly notes with this wording until we get a better one and probably a SUMO page explaining the feature we could link to:
Added Touch Bar support on macOS

Comment 127

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/projects/cedar/rev/099f631b90046b102aa1868455f03b7f9942eaba
Bug 1313429 - Add Touch Bar functionality to Firefox r=spohl,mikedeboer,flod

Comment 128

[Sylvestre Ledru [:Sylvestre]]

Coverity is complaining about:
CID 19373 (#1 of 1): Missing break in switch (MISSING_BREAK)unterminated_case: The case for value "intl:app-locales-changed" is not terminated by a 'break' statement.
here:
https://searchfox.org/mozilla-central/source/browser/components/touchbar/MacTouchBar.js#310
is that expected?

Comment 129

[Stephen A Pohl]

(In reply to Sylvestre Ledru [:sylvestre] from comment #128)

Coverity is complaining about:
CID 19373 (#1 of 1): Missing break in switch (MISSING_BREAK)unterminated_case: The case for value "intl:app-locales-changed" is not terminated by a 'break' statement.
here:
https://searchfox.org/mozilla-central/source/browser/components/touchbar/MacTouchBar.js#310
is that expected?

Definitely not! Luckily, locales don't change too frequently but we should fix this.

Comment 130

[Stephen A Pohl]

Attachment: Followup to address comment 128

Comment 131

[Mike de Boer [:mikedeboer]]

Comment on attachment 9038661 [details] [diff] [review]
Followup to address comment 128

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

Thanks!

Comment 132

[Harry Twyford]

Thank you for covering this spohl!

Comment 133

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/143af10f6e44
Followup to add a missing break for a case in a switch statement (discovered by coverity). r=mikedeboer

Comment 134

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/143af10f6e44

Comment 135

[Liz Henry (:lizzard)]

Adding a release note, currently just "Initial macOS touchbar support".

Tim, or Mike, or anyone really: Can you suggest better wording with more detail? Or, is there anything you can link to that explains further?

Comment 136

[Tim Nguyen :ntim]

"Basic support for macOS touchbar" would probably work, although I have to admit I'm not an expert in wording.

As for links, the best I have is a reddit post: https://www.reddit.com/r/firefox/comments/aitx1q/psa_macos_touch_bar_support_is_in_the_latest/

I don't think that's a good official resource though.

Comment 137

[Liz Henry (:lizzard)]

This might be good to include at https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/66.

Comment 138

[Mike de Boer [:mikedeboer]]

I don't think there's anything to report for developers. Once we add a WebExtension API to control the touchbar, we should, but not for now.

Comment 140

[Harry Twyford]

I'm removing the alias so the new Touch Bar meta (bug 1603568) can use it.