914921 - australis menu support for status button

Status: VERIFIED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Shane Caraveo (:mixedpuppy)]

Attachment: WIP hamburger.patch

status and mark buttons need to support being placed into the hamburger.  WIP patch makes progress towards that.  The status button works well, the mark buttons less so, but it is in a state that some basic feedback would be worthwhile to make sure it's on the right track in usage of the customizable ui stuff.

issues to resolve:
- the mark button seems to get "lost" sometimes and wont come back even on restart, even though it is created and placed into the palette.  (I need to update my branch after the recent placement fixes, that is probably the issue)
- mark button doesn't work quite right after transition from toolbar to menu
- notifications for status buttons dont bubble up yet

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 802699 [details] [diff] [review]
WIP hamburger.patch

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

I admit to not understanding alot of this and it's untested, but there's no huge issues I can see.  I look forward to Gijs' feedback :)

::: browser/base/content/browser-social.js
@@ +1553,5 @@
> +    let customized = widgetGroup.areaType == CustomizableUI.TYPE_MENU_PANEL;
> +    if (customized) {
> +      panel = document.getElementById("PanelUI-socialapi");
> +      this._attachNotificatonPanel(panel, aToolbarButton, provider);
> +      widget.node.setAttribute("noautoclose", "true");

does this attribute need to be removed in the else?

@@ +1662,5 @@
>  SocialMarks = {
>    update: function() {
>      // signal each button to update itself
>      let currentButtons = document.querySelectorAll('toolbarbutton[type="socialmark"]');
> +    dump("socialmarks updating buttons "+currentButtons.length+"\n");

stating the obvious, but these dumps must go before review.

@@ +1766,5 @@
>      let palette = document.getElementById("navigator-toolbox").palette;
>      let button = document.createElement("toolbarbutton");
>      button.setAttribute("type", "socialmark");
>      button.setAttribute("class", "toolbarbutton-1 social-mark-button");
> +    button.setAttribute("image", provider.iconURL);

aren't we going to want listStyleImage() in the non-hamburger case?

@@ +1771,3 @@
>      button.setAttribute("origin", provider.origin);
>      button.setAttribute("id", this._toolbarHelper.idFromOrgin(provider.origin));
> +    button.setAttribute("oncommand", "this.markCurrentPage();");

how did the command get executed before this patch?

::: browser/base/content/browser.css
@@ +685,5 @@
>  
> +panelview > .social-panel-frame {
> +  width: auto;
> +  height: auto;
> +  border: 1px solid red;

where did the need for the 1px red border come from?

::: browser/base/content/socialmarks.xml
@@ +7,5 @@
>  
>  
>    <binding id="toolbarbutton-marks" display="xul:button"
>             extends="chrome://global/content/bindings/toolbarbutton.xml#toolbarbutton">
> +    <content xdisabled="true">

what's the intent here with 'xdisabled'?

@@ +22,5 @@
> +          let widgetGroup = CustomizableUI.getWidget(this.getAttribute("id"));
> +          let widget = widgetGroup.forWindow(window);
> +          this.customized = widgetGroup.areaType == CustomizableUI.TYPE_MENU_PANEL;
> +          if (this.customized) {
> +            widget.node.setAttribute("noautoclose", "true");

similar to before - should this be removed in the "else" case?

Comment 2

[:Gijs (he/him)]

Comment on attachment 802699 [details] [diff] [review]
WIP hamburger.patch

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

You still don't seem to have marked the social-toolbar-item as being removable in browser.xul? I also don't see the mark button as being created as removable, but maybe that's already the case?

This is mostly a bit of a cursory glance (as per 'feedback' rather than 'review' :-) ). I'm not very familiar with the social API code yet. Here are some comments anyway! :-)

(feedback- because of the problems I foresee when the iframe gets reinitialized and load handlers get lost)

::: browser/base/content/browser-social.js
@@ +1500,5 @@
>      } else {
> +      // reparent the iframe if the button has been customized to a different
> +      // location
> +      if (frame.parentNode != aParent)
> +        aParent.appendChild(frame);

Note that if this is a genuine frame with separate content (which I think it is?) then it'll get reinitialized if you do this - i.e. content will be reloaded - in asynchronous fashion. Is that OK?

@@ +1549,5 @@
> +    // if we're a slice in the hambuger, use that panel instead
> +    let widgetGroup = CustomizableUI.getWidget(aToolbarButton.getAttribute("id"));
> +    let widget = widgetGroup.forWindow(window);
> +    let panel, showingEvent, hidingEvent;
> +    let customized = widgetGroup.areaType == CustomizableUI.TYPE_MENU_PANEL;

This means customized will be false if the item is not present at all, or if it's in another toolbar. From the rest of the code, I *think* this will be OK... but have you thought about those cases? In any case, personally I would advocate for a different variable name, e.g. "inPanel" or something.

@@ +1582,5 @@
>        evt.initCustomEvent(name, true, true, {});
>        notificationFrame.contentDocument.documentElement.dispatchEvent(evt);
>      }
>  
> +    let dynamicResizer = customized ? null : this._dynamicResizer;

Not familiar with what the resizer does - why do you not need it for the panel case?

::: browser/base/content/browser.css
@@ +685,5 @@
>  
> +panelview > .social-panel-frame {
> +  width: auto;
> +  height: auto;
> +  border: 1px solid red;

Also not quite clear on this. I imagine it's for debugging?

::: browser/base/content/socialmarks.xml
@@ +14,5 @@
>        <xul:label class="toolbarbutton-text" crop="right" flex="1"
>                   xbl:inherits="value=label,accesskey,crop"/>
>      </content>
>      <implementation implements="nsIDOMEventListener, nsIObserver">
> +      <field name="customized">true</field>

Same comment regarding naming.

@@ +61,5 @@
> +          //                                           null, null, null, null);
> +          //  let panel = aNotificationFrame.parentNode;
> +          //  if (!this.customized)
> +          //    sizeSocialPanelToContent(panel, aNotificationFrame);
> +          //});

Commented out code is sad. Why is this commented out?

@@ +229,5 @@
> +          let evName = customized ? "ViewHiding": "popuphidden";
> +          panel.addEventListener(evName, function _hidden() {
> +            panel.removeEventListener(evName, _hidden);
> +            this.update();
> +          }.bind(this), false);

Same removal problem here (see below; sorry, got to this hunk later).

@@ +299,1 @@
>            }.bind(this), true);

NB: when you reparent the iframe, there will be a new content document. You might still have a load handler attached to the old one. You should probably avoid that happening, but I don't know what the best way to do that would be, as I don't pretend to understand all of the socialAPI code.

Also, the removal code in this block is broken (and was in the original revision, too), because if you do:

function foo() { }.bind(this)

foo != foo.bind(this)

and so you're not trying to remove the same function you're adding.

This is a pattern I've seen too often in the last few months, and I'm actually starting to think of writing an automatic checker for it and audit browser/, toolkit/ etc... The consequences here are hopefully not too bad, but they can be a lot worse (e.g. leaking the world).

@@ +316,5 @@
> +            if (!link || !link.ownerDocument || !rel || !link.href)
> +              return;
> +            if (link.rel.indexOf("icon") < 0)
> +              return;
> +    

Nit: whitespace

@@ +320,5 @@
> +    
> +            let uri = DOMLinkHandler.getLinkIconURI(link);
> +            if (!uri)
> +              return;
> +    

Nit: whitespace

Comment 3

[Shane Caraveo (:mixedpuppy)]

Finally getting back to this.


(In reply to :Gijs Kruitbosch from comment #2)
> Comment on attachment 802699 [details] [diff] [review]
> WIP hamburger.patch
> 
> Review of attachment 802699 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You still don't seem to have marked the social-toolbar-item as being
> removable in browser.xul? 

I am not making that button removable, the interactions with the sidebar are quite complex.  This patch is about making the SocialStatus and SocialMark buttons work in the australis menu.

> I also don't see the mark button as being created
> as removable, but maybe that's already the case?

I thought it was by default removable.  At least in manual testing, it is.

> This is mostly a bit of a cursory glance (as per 'feedback' rather than
> 'review' :-) ). I'm not very familiar with the social API code yet. Here are
> some comments anyway! :-)

exactly what I was asking for  :)

> (feedback- because of the problems I foresee when the iframe gets
> reinitialized and load handlers get lost)
> 
> ::: browser/base/content/browser-social.js
> @@ +1500,5 @@
> >      } else {
> > +      // reparent the iframe if the button has been customized to a different
> > +      // location
> > +      if (frame.parentNode != aParent)
> > +        aParent.appendChild(frame);
> 
> Note that if this is a genuine frame with separate content (which I think it
> is?) then it'll get reinitialized if you do this - i.e. content will be
> reloaded - in asynchronous fashion. Is that OK?

Yes, that is ok.

> @@ +1549,5 @@
> > +    // if we're a slice in the hambuger, use that panel instead
> > +    let widgetGroup = CustomizableUI.getWidget(aToolbarButton.getAttribute("id"));
> > +    let widget = widgetGroup.forWindow(window);
> > +    let panel, showingEvent, hidingEvent;
> > +    let customized = widgetGroup.areaType == CustomizableUI.TYPE_MENU_PANEL;
> 
> This means customized will be false if the item is not present at all, or if
> it's in another toolbar. From the rest of the code, I *think* this will be
> OK... but have you thought about those cases? In any case, personally I
> would advocate for a different variable name, e.g. "inPanel" or something.

I'll update the name.   This code is in sections that will only happen if the buttons are in the UI (ie. this is the showPopup code path that happens on the button oncommand or click)

> @@ +1582,5 @@
> >        evt.initCustomEvent(name, true, true, {});
> >        notificationFrame.contentDocument.documentElement.dispatchEvent(evt);
> >      }
> >  
> > +    let dynamicResizer = customized ? null : this._dynamicResizer;
> 
> Not familiar with what the resizer does - why do you not need it for the
> panel case?

The iframe flexes to the size of the menu panel, so we will not allow resizing there, at least in this iteration.  That will require some new documentation for providers, which is fine since both these buttons are new.

> ::: browser/base/content/browser.css
> @@ +685,5 @@
> >  
> > +panelview > .social-panel-frame {
> > +  width: auto;
> > +  height: auto;
> > +  border: 1px solid red;
> 
> Also not quite clear on this. I imagine it's for debugging?

For the border, yes.

> @@ +61,5 @@
> > +          //                                           null, null, null, null);
> > +          //  let panel = aNotificationFrame.parentNode;
> > +          //  if (!this.customized)
> > +          //    sizeSocialPanelToContent(panel, aNotificationFrame);
> > +          //});
> 
> Commented out code is sad. Why is this commented out?

my brain has bitrotted on why, so I'll have to look back at it and either remove it entirely or fix it.

> @@ +229,5 @@
> > +          let evName = customized ? "ViewHiding": "popuphidden";
> > +          panel.addEventListener(evName, function _hidden() {
> > +            panel.removeEventListener(evName, _hidden);
> > +            this.update();
> > +          }.bind(this), false);
> 
> Same removal problem here (see below; sorry, got to this hunk later).
> 
> @@ +299,1 @@
> >            }.bind(this), true);
> 
> NB: when you reparent the iframe, there will be a new content document. You
> might still have a load handler attached to the old one. You should probably
> avoid that happening, but I don't know what the best way to do that would
> be, as I don't pretend to understand all of the socialAPI code.
> 
> Also, the removal code in this block is broken (and was in the original
> revision, too), because if you do:
> 
> function foo() { }.bind(this)
> 
> foo != foo.bind(this)
> 
> and so you're not trying to remove the same function you're adding.
> 
> This is a pattern I've seen too often in the last few months, and I'm
> actually starting to think of writing an automatic checker for it and audit
> browser/, toolkit/ etc... The consequences here are hopefully not too bad,
> but they can be a lot worse (e.g. leaking the world).

I'll re-examine the removal of the iframe.

Comment 4

[:Gijs (he/him)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #3)
> Finally getting back to this.
> 
> 
> (In reply to :Gijs Kruitbosch from comment #2)
> > Comment on attachment 802699 [details] [diff] [review]
> > WIP hamburger.patch
> > 
> > Review of attachment 802699 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > You still don't seem to have marked the social-toolbar-item as being
> > removable in browser.xul? 
> 
> I am not making that button removable, the interactions with the sidebar are
> quite complex.  This patch is about making the SocialStatus and SocialMark
> buttons work in the australis menu.

Note that buttons need to be removable in order to be able to move them out of their container using the CustomizableUI APIs or UI. That's what the removable attribute means. I guess what you mean is that the button shouldn't be able to be removed entirely, but that's not currently something you can do. Instead, you probably want to use the CustomizableUI APIs to detect that the button is being removed, and disable the provider at that point? OTOH, note that you'll get a remove notification even if someone moves a button from one area to another, as it's first removed from the first area and then being added to the next one.

Comment 5

[Shane Caraveo (:mixedpuppy)]

Attachment: menu support for SocialStatus

This removes the work on SocialMarks buttons and focuses solely on the Status button.  It should simplify review somewhat since the SocialMarks implementation will require slightly more exhausting changes.

Comment 6

[:Gijs (he/him)]

Comment on attachment 816576 [details] [diff] [review]
menu support for SocialStatus

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

This looks alright generally speaking. You may want to look into the createWidget API and whether that'd let you simplify some of your current code. You'd essentially want to create the widget, and the CustomizableUI APIs will take care of having it appear in the right place, having the panel attached to it, etc.

The only other thing I'm missing is code to deal with what happens if the node goes into the palette (ie, is in *neither* the menu panel or a toolbar).

::: browser/base/content/browser-social.js
@@ +1563,5 @@
> +    } else {
> +      panel = document.getElementById("social-notification-panel");
> +      this._attachNotificatonPanel(panel, aToolbarButton, provider);
> +      showingEvent = "popupshown";
> +      hidingEvent = "popuphidden";

Have you thought about what should happen if the user opens customization mode and drags the item to the palette?

@@ +1624,2 @@
>            dispatchPanelEvent("socialFrameShow");
>          }, true);

You can probably pull out a function here to reduce the duplication.

@@ +1627,5 @@
>      });
>  
> +    if (inMenuPanel) {
> +      PanelUI.showSubView("PanelUI-socialapi", widget.node,
> +                          CustomizableUI.AREA_PANEL);

Actually, this method should work when the node is in the toolbar, too. It'll create a temporary container to show your view in.

You could even look into creating a type: "view" widget using the CustomizableUI.createWidget API, and letting that take care of showing the item's panel when required.

Comment 7

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 816576 [details] [diff] [review]
menu support for SocialStatus

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

Clearing review request pending a refreshed patch addressing Gijs' comments - particularly his comment about items being explicitly dragged to the palette.

Comment 8

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gijs Kruitbosch from comment #6)
> The only other thing I'm missing is code to deal with what happens if the
> node goes into the palette (ie, is in *neither* the menu panel or a toolbar).
> 
> ::: browser/base/content/browser-social.js
> @@ +1563,5 @@
> > +    } else {
> > +      panel = document.getElementById("social-notification-panel");
> > +      this._attachNotificatonPanel(panel, aToolbarButton, provider);
> > +      showingEvent = "popupshown";
> > +      hidingEvent = "popuphidden";
> 
> Have you thought about what should happen if the user opens customization
> mode and drags the item to the palette?

That particular code should only ever get executed via oncommand, which iiuc can not happen when the button is in the palette.  The new patch does address a couple items when moving the button into/out of the palette.

> @@ +1627,5 @@
> >      });
> >  
> > +    if (inMenuPanel) {
> > +      PanelUI.showSubView("PanelUI-socialapi", widget.node,
> > +                          CustomizableUI.AREA_PANEL);
> 
> Actually, this method should work when the node is in the toolbar, too.
> It'll create a temporary container to show your view in.

I tried that and it didn't work.

> You could even look into creating a type: "view" widget using the
> CustomizableUI.createWidget API, and letting that take care of showing the
> item's panel when required.

I also did a quick investigation on using view.  While I think we could do that with other changes, I think this should be fixed sooner than later, and a follow up to integrate these buttons even further into the widget system.  The button does now use the widget system since bug 940820.

Comment 9

[Shane Caraveo (:mixedpuppy)]

Attachment: menu support for SocialStatus

Comment 10

[Shane Caraveo (:mixedpuppy)]

https://tbpl.mozilla.org/?tree=Try&rev=66ca9a3d7e51

Comment 11

[:Gijs (he/him)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #8)
> > @@ +1627,5 @@
> > >      });
> > >  
> > > +    if (inMenuPanel) {
> > > +      PanelUI.showSubView("PanelUI-socialapi", widget.node,
> > > +                          CustomizableUI.AREA_PANEL);
> > 
> > Actually, this method should work when the node is in the toolbar, too.
> > It'll create a temporary container to show your view in.
> 
> I tried that and it didn't work.

I hate to be stop energy here, but we should really figure out why this is rather than working around it. I worked around lacking APIs in jetpack last month and the result is bugs still biting us now. If this is a bug in PanelUI, it needs to be fixed.

Can you provide more detail about "didn't work"? What happened? Just nothing showing up, or something else? Any errors in the error console?

Comment 12

[:Gijs (he/him)]

Comment on attachment 8337136 [details] [diff] [review]
menu support for SocialStatus

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

::: browser/base/content/browser-social.js
@@ +1396,5 @@
> +          onWidgetRemoved: function(aWidgetId, aPrevArea) {
> +            if (aWidgetId != node.id)
> +              return;
> +            //// When a widget is demoted to the palette ('removed'), it's visual
> +            //// style should change.

Nit: no need for four slashes :-)

@@ +1399,5 @@
> +            //// When a widget is demoted to the palette ('removed'), it's visual
> +            //// style should change.
> +            SocialStatus.updateNotification(provider.origin);
> +            // We also remove the iframe for this button
> +            let notificationFrameId = "social-status-" + provider.origin;

Shouldn't this use the same utility function as the other patch I reviewed recently? Otherwise this might get out of sync. Maybe I'm missing something though!

@@ +1411,5 @@
> +            if (aWidgetId != node.id || aDoc != document)
> +              return;
> +            CustomizableUI.removeListener(listener);
> +          },
> +          onWidgetReset: function(aWidgetId) {},

You don't need to define listeners you're not using.

@@ +1415,5 @@
> +          onWidgetReset: function(aWidgetId) {},
> +          onWidgetMoved: function(aWidgetId, aArea) {},
> +          onWidgetDrag: function(aWidgetId, aArea) {}
> +        };
> +        CustomizableUI.addListener(listener);

So now you're adding one listener for each document you're changing. You wrote about a followup bug, which makes sense, but I'll note here in case you want to fix it here, that AFAICT you should be able to just have one listener.

::: browser/base/content/browser.css
@@ +745,5 @@
> +  max-width: 32px;
> +  max-height: 32px;
> +}
> +
> +@media not all and (max-resolution: 1dppx) {

This is a peculiar media query to use... What are you trying to achieve, and why not just use 2dppx if this is for mac stuff? :-)

Comment 13

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8337136 [details] [diff] [review]
menu support for SocialStatus

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

Cancelling review pending a new version with Gij's comments addressed.

Comment 14

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gijs Kruitbosch from comment #12)
> Comment on attachment 8337136 [details] [diff] [review]
> menu support for SocialStatus

> @@ +1399,5 @@
> > +            //// When a widget is demoted to the palette ('removed'), it's visual
> > +            //// style should change.
> > +            SocialStatus.updateNotification(provider.origin);
> > +            // We also remove the iframe for this button
> > +            let notificationFrameId = "social-status-" + provider.origin;
> 
> Shouldn't this use the same utility function as the other patch I reviewed
> recently? Otherwise this might get out of sync. Maybe I'm missing something
> though!

That id is specific to the iframe used for social panels, there shouldn't be any interaction there with the widget code.

> @@ +1415,5 @@
> > +          onWidgetReset: function(aWidgetId) {},
> > +          onWidgetMoved: function(aWidgetId, aArea) {},
> > +          onWidgetDrag: function(aWidgetId, aArea) {}
> > +        };
> > +        CustomizableUI.addListener(listener);
> 
> So now you're adding one listener for each document you're changing. You
> wrote about a followup bug, which makes sense, but I'll note here in case
> you want to fix it here, that AFAICT you should be able to just have one
> listener.

I followed the pattern used in CustomizableWidgets.jsm where buttons are adding listeners for themselves in either onBuild or onCreated.  Is this generally wrong?

Comment 15

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gijs Kruitbosch from comment #11)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #8)
> > > @@ +1627,5 @@
> > > >      });
> > > >  
> > > > +    if (inMenuPanel) {
> > > > +      PanelUI.showSubView("PanelUI-socialapi", widget.node,
> > > > +                          CustomizableUI.AREA_PANEL);
> > > 
> > > Actually, this method should work when the node is in the toolbar, too.
> > > It'll create a temporary container to show your view in.
> > 
> > I tried that and it didn't work.
> 
> I hate to be stop energy here, but we should really figure out why this is
> rather than working around it. I worked around lacking APIs in jetpack last
> month and the result is bugs still biting us now. If this is a bug in
> PanelUI, it needs to be fixed.
> 
> Can you provide more detail about "didn't work"? What happened? Just nothing
> showing up, or something else? Any errors in the error console?

Sorry, it's not that the panel doesn't appear or that technically it doesn't work, it's that the social panels have a couple of characteristics that the widget panel doesn't support right now, and that is perhaps a bigger effort that could be handled later.

The primary issues are:

- social panels have a different styling, primarily 0 margin/padding.
- social panels resize based on content loaded in the attached iframe.

This patch gets the button working, with the "work around" code being stuff that has been working for a long time.  I do want to move more towards a pure widget, it just will take more time and I'd like to get the social buttons to a working state.

Comment 16

[Shane Caraveo (:mixedpuppy)]

Attachment: menu support for SocialStatus

Comment 17

[:Gijs (he/him)]

Comment on attachment 8337502 [details] [diff] [review]
menu support for SocialStatus

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

Otherwise this looks alright, CustomizableUI-code-wise.

Comment 18

[:Gijs (he/him)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #14)
> > @@ +1415,5 @@
> > > +          onWidgetReset: function(aWidgetId) {},
> > > +          onWidgetMoved: function(aWidgetId, aArea) {},
> > > +          onWidgetDrag: function(aWidgetId, aArea) {}
> > > +        };
> > > +        CustomizableUI.addListener(listener);
> > 
> > So now you're adding one listener for each document you're changing. You
> > wrote about a followup bug, which makes sense, but I'll note here in case
> > you want to fix it here, that AFAICT you should be able to just have one
> > listener.
> 
> I followed the pattern used in CustomizableWidgets.jsm where buttons are
> adding listeners for themselves in either onBuild or onCreated.  Is this
> generally wrong?

I'm not a fan, yeah. I guess I should file a bug about this at some point... :-\

(In reply to Shane Caraveo (:mixedpuppy) from comment #15)
> Sorry, it's not that the panel doesn't appear or that technically it doesn't
> work, it's that the social panels have a couple of characteristics that the
> widget panel doesn't support right now, and that is perhaps a bigger effort
> that could be handled later.
> 
> The primary issues are:
> 
> - social panels have a different styling, primarily 0 margin/padding.
> - social panels resize based on content loaded in the attached iframe.
> 
> This patch gets the button working, with the "work around" code being stuff
> that has been working for a long time.  I do want to move more towards a
> pure widget, it just will take more time and I'd like to get the social
> buttons to a working state.

OK, that puts me more at ease with doing things this way. I do wonder though, what happens if you move the button to the menu panel and the view shows as a subview inside the main menupanel? Do you resize the entire menupanel based on the Social API node?

Comment 19

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gijs Kruitbosch from comment #18)
> > The primary issues are:
> > 
> > - social panels have a different styling, primarily 0 margin/padding.
> > - social panels resize based on content loaded in the attached iframe.
> > 
> > This patch gets the button working, with the "work around" code being stuff
> > that has been working for a long time.  I do want to move more towards a
> > pure widget, it just will take more time and I'd like to get the social
> > buttons to a working state.
> 
> OK, that puts me more at ease with doing things this way. I do wonder
> though, what happens if you move the button to the menu panel and the view
> shows as a subview inside the main menupanel? Do you resize the entire
> menupanel based on the Social API node?

When the social iframe is inside the panelview, it sizes to the panelview rather than resizing the panel.  Social providers will have to adjust to this in their code since the menu is often smaller than the panels in our buttons.

Comment 20

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8337502 [details] [diff] [review]
menu support for SocialStatus

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

It seems onBuild should not be in a <script> attached to a window, but in a .jsm (or component).  Shane says he's reworking some of this, so cancelling request.

Comment 21

[Shane Caraveo (:mixedpuppy)]

Attachment: menu support for SocialStatus

This version moves the widgets into Social.jsm and makes the widget listener per-window rather than per-button.  That might be able to be made per-process at a later time, but needs access to some chrome classes right now.

Comment 22

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8339705 [details] [diff] [review]
menu support for SocialStatus

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

Looks OK to me.

::: browser/base/content/browser-social.js
@@ +1309,4 @@
>    this._createButton = createButtonFn;
>    this._type = type;
> +
> +  if (listener) {

I'm not too bothered about this, but I don't really understand why this is in the Toolbar helper, given only 1 of the 2 users of the object uses it.  Wouldn't it be better down where the listener is passed in?

@@ +1515,2 @@
>  
> +    // if we're a slice in the hambuger, use that panel instead

typo - hamburger

::: browser/base/content/browser.css
@@ +728,5 @@
>    -moz-binding: url("chrome://browser/content/urlbarBindings.xml#toolbarbutton-badged");
>  }
>  
> +toolbarpaletteitem[place="palette"] > toolbarbutton[type="badged"] > .toolbarbutton-badge-container {
> +  padding: 5px;

I'm glossing over these css changes, but if they are the same as what Gijs' previously f+'d, that's fine.

::: browser/modules/Social.jsm
@@ +384,5 @@
>  function schedule(callback) {
>    Services.tm.mainThread.dispatch(callback, Ci.nsIThread.DISPATCH_NORMAL);
>  }
>  
> +function CreateSocialStatusWidget(aId, aProvider) {

If this code is almost identical to Gijs' f+, then that's fine, but otherwise please re-request feedback from him.

@@ +401,5 @@
> +    removable: true,
> +    defaultArea: CustomizableUI.AREA_NAVBAR,
> +    onBuild: function(aDocument) {
> +      let window = aDocument.defaultView;
> +  

a few trailing whitespaces here and lower

Comment 23

[Shane Caraveo (:mixedpuppy)]

(In reply to Mark Hammond [:markh] from comment #22)
> Comment on attachment 8339705 [details] [diff] [review]
> menu support for SocialStatus
> 
> Review of attachment 8339705 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks OK to me.
> 
> ::: browser/base/content/browser-social.js
> @@ +1309,4 @@
> >    this._createButton = createButtonFn;
> >    this._type = type;
> > +
> > +  if (listener) {
> 
> I'm not too bothered about this, but I don't really understand why this is
> in the Toolbar helper, given only 1 of the 2 users of the object uses it. 
> Wouldn't it be better down where the listener is passed in?

The SocialMark class will end up using a listener as well, but that work will happen in bug 940155.  When I get to that, I may refactor this depending on how things work out.

Comment 24

[Shane Caraveo (:mixedpuppy)]

Attachment: menu support for SocialStatus

https://tbpl.mozilla.org/?tree=Try&rev=073aa640e040

Comment 25

[Shane Caraveo (:mixedpuppy)]

Attachment: menu support for SocialStatus

new try https://tbpl.mozilla.org/?tree=Try&rev=88be84968dc8

Trying to get a widget in the widget listener proves to be a bad thing.  Instead, we rely on the fact that the button id and widget id are the same.  Also change form close to unload for removing the listener, since close can be canceled.

Comment 26

[Shane Caraveo (:mixedpuppy)]

Attachment: menu support for SocialStatus

verification try after patch updated with feedback: https://tbpl.mozilla.org/?tree=Try&rev=9bfb7e708147

https://hg.mozilla.org/integration/fx-team/rev/89107399e8db

Comment 27

[Shane Caraveo (:mixedpuppy)]

> verification try after patch updated with feedback:
> https://tbpl.mozilla.org/?tree=Try&rev=9bfb7e708147

nix that, mixing bugs/patches, try is for something else, previous try was in comment 25 is for this patch

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/89107399e8db

Comment 29

[Cornel Ionce [:noni] [Hubs QA]]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:29.0) Gecko/20100101 Firefox/29.0

Verified fixed on latest Aurora (build ID: 20140310004003).