Closed Bug 1023304 Opened 6 years ago Closed 6 years ago

Make it possible to auto-add new built-in widgets to already customized areas

Categories

(Firefox :: Toolbars and Customization, defect)

x86
All
defect
Not set
Points:
2

Tracking

()

VERIFIED FIXED
Firefox 33
Iteration:
34.1

People

(Reporter: mconley, Assigned: Gijs)

References

Details

Attachments

(1 file, 1 obsolete file)

This code kinda spells it out:

http://hg.mozilla.org/mozilla-central/annotate/9dc0ffca10f4/browser/components/customizableui/src/CustomizableUI.jsm#l272

The result being that for any areas that a user has customized, there's no nice and easy API for ensuring that _new_ widgets that we develop (like the Loop button, for example) are auto-added to those areas.

We used to do this with an nsBrowserGlue.js migration, and I suppose we could do the same thing for CustomizableUI by manually mucking about with the customization state saved in prefs, but we probably want something a little less hackish.
Flags: firefox-backlog+
Cc'ing some relevant folks.

I _believe_ this is blocking Loop (bug 1023193), so we might want someone to grab this in a near-future iteration.
So I think we do a better job of supporting addons here - code that calls CUI.createWidget outside of CustomizableWidgets.jsm are auto-added to customized areas; however, they'll be considered add-on buttons, and they'll get flushed out during a restoration of the default state (if the user clicks "Restore Defaults" in customization mode).
Yeah, so, right now:
- you don't get nice things if you're a XUL widget
- if you're an API widget you want to be in CustomizableWidgets.jsm because otherwise because of bug 982656 you won't be reset to the right place (although I guess if you're builtin you can workaround by adjusting the actual defaultPlacements of the relevant area (!))
- if you're an API widget inside CustomizableWidgets.jsm, we don't insert you in the right place.

Damned if you do, damned if you don't, in other words.

The other thing is that I believe we currently just append new items to the end, and I expect that consumers will actually want to be inserted before 'some item X'.
It sounds like the Loop folks really need this. What are the chances this can get picked up for the next iteration?
Flags: needinfo?(gijskruitbosch+bugs)
Lemme redirect this question to GMC, actually.

GMC: it sounds as if this bug is needed for Loop to get good exposure (so that existing profiles can have the Loop button included in their UI). Assuming that's a thing we all agree we want to do, is it possible to get this added to a future iteration?

(I'm not working on Loop or anything, but I know Loop folk, and it sounds as if they need this.)
Flags: needinfo?(madhava)
Flags: needinfo?(gavin.sharp)
Flags: needinfo?(cweiner)
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(gavin.sharp) → needinfo?(dolske)
(In reply to Mike Conley (:mconley) from comment #5)
 
> GMC: it sounds as if this bug is needed for Loop to get good exposure (so
> that existing profiles can have the Loop button included in their UI).
> Assuming that's a thing we all agree we want to do, is it possible to get
> this added to a future iteration?
> 

Thanks, Mike.  We're going to add this to this bug to the next iteration.  We agree it's important, especially in pre-release to increase exposure of Loop so we can assess how to use varying degrees of exposure as we move towards release.
Flags: needinfo?(cweiner)
Flags: needinfo?(madhava)
Flags: needinfo?(dolske)
I can work on this this week, starting either today or tomorrow.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 33.3
Points: --- → 2
Added to the priority list for Iteration 33.3
Hi Gijs, can you mark this bug as [qa+] or [qa-] for verification.
QA Whiteboard: [qa?]
Flags: needinfo?(gijskruitbosch+bugs)
QA Whiteboard: [qa?] → [qa+]
Flags: needinfo?(gijskruitbosch+bugs)
This is trickier than I thought. My initial plan was to auto-add things that weren't in gSeenWidgets, much like we do for add-on widgets. Then I realized that we don't track builtin widgets in gSeenWidgets - only things that go through createWidget end up in there. I could fix that, of course.

However, then we end up (a) bloating gSeenWidgets (keep in mind all of this is stuck in a pref, and we'll basically be doubling its size), and (b) this is going to auto-add ALL the default widgets back to their default area, unless I start adding some way to distinguish the existing ones from Loop / whatever else we're newly adding.

As I'm writing this comment, I'm thinking maybe the best thing would be to make things happen in nsBrowserGlue (shudder) or implement a similar-but-better migration thing with versioned saved states for CUI?

Blair: halp? :-)
Attachment #8452706 - Flags: feedback?(bmcbride)
I like this better. Tested by chucking an extra widget in at the end of customizablewidgets, and checked that this worked for toolbars and the panel. Sadly, I don't think we can really create an automated test for this... I would have used the loop button, but that's still implemented as a xul widget... :-/
Attachment #8453410 - Flags: review?(bmcbride)
Attachment #8452706 - Attachment is obsolete: true
Attachment #8452706 - Flags: feedback?(bmcbride)
Comment on attachment 8453410 [details] [diff] [review]
auto-add new builtin widgets in customizable areas,

Mike, if you're comfortable reviewing this and have time to look, could you? I'm guessing the Loop people will want this for 33 still.
Attachment #8453410 - Flags: review?(mconley)
Comment on attachment 8453410 [details] [diff] [review]
auto-add new builtin widgets in customizable areas,

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

This looks fine to me. Too bad about no automated tests, but yeah, I can see that being pretty hard here. Assuming all pre-existing customizableui tests pass, r=me.

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +149,5 @@
>  
>      this.addListener(this);
>      this._defineBuiltInWidgets();
>      this.loadSavedState();
> +    this._introduceNewBuiltinWidgets();

Semi-related nit: I'm curious as to why any of these methods are _ prefixed. I mean, the whole thing is private unless explicitly called into from CustomizableUI...
Attachment #8453410 - Flags: review?(mconley) → review+
(In reply to Mike Conley (:mconley) from comment #13)
> Comment on attachment 8453410 [details] [diff] [review]
> auto-add new builtin widgets in customizable areas,
> 
> Review of attachment 8453410 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks fine to me. Too bad about no automated tests, but yeah, I can see
> that being pretty hard here. Assuming all pre-existing customizableui tests
> pass, r=me.
> 
> ::: browser/components/customizableui/src/CustomizableUI.jsm
> @@ +149,5 @@
> >  
> >      this.addListener(this);
> >      this._defineBuiltInWidgets();
> >      this.loadSavedState();
> > +    this._introduceNewBuiltinWidgets();
> 
> Semi-related nit: I'm curious as to why any of these methods are _ prefixed.
> I mean, the whole thing is private unless explicitly called into from
> CustomizableUI...

You can technically get to it using the backstagepass that Cu.import produces... not that that'd be a good idea. But yes, these are largely academic, although I would still argue that they at least indicate that they shouldn't be called directly from CustomizableUI itself.
remote:   https://hg.mozilla.org/integration/fx-team/rev/2cfd5b1c4759



This isn't really testable, so setting qa-...
QA Whiteboard: [qa+] → [qa-]
https://hg.mozilla.org/mozilla-central/rev/2cfd5b1c4759
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Comment on attachment 8453410 [details] [diff] [review]
auto-add new builtin widgets in customizable areas,

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

Sounds good.
Attachment #8453410 - Flags: review?(bmcbride) → review+
Though, I am worried about having no test coverage - not even manual coverage. Can we not test this via calling APIs via BackstagePass, to forcibly add a new builtin widget and then fake part of the initialization step again by manually calling introduceNewBuiltinWidgets() and checking gFuturePlacements (which is already tested)? It's not perfect, but it'd be far better than nothing.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Blair McBride [:Unfocused] from comment #18)
> Though, I am worried about having no test coverage - not even manual
> coverage. Can we not test this via calling APIs via BackstagePass, to
> forcibly add a new builtin widget and then fake part of the initialization
> step again by manually calling introduceNewBuiltinWidgets() and checking
> gFuturePlacements (which is already tested)? It's not perfect, but it'd be
> far better than nothing.

This is a good suggestion. I'll try to get something together later today.
Status: RESOLVED → REOPENED
Flags: needinfo?(gijskruitbosch+bugs)
Resolution: FIXED → ---
Attachment #8453410 - Flags: checkin+
Status: REOPENED → ASSIGNED
Iteration: 33.3 → 34.1
Depends on: 1042100
Resolving and moving the test issue to bug 1042100. There's nothing QA can do here, so I'll mark this as verified, too.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.