Closed Bug 936195 Opened 7 years ago Closed 7 years ago

UITour: Give webpage a way to query what UI targets are available

Categories

(Firefox :: General, defect, P3)

defect

Tracking

()

VERIFIED FIXED
Firefox 30
Tracking Status
firefox29 --- fixed
firefox30 --- fixed

People

(Reporter: Unfocused, Assigned: adw)

References

(Depends on 1 open bug)

Details

(Whiteboard: [Australis:M-])

Attachments

(2 files, 1 obsolete file)

Currently, when a tour webpage requests a highlight/info panel for a UI element that has been removed via customization, we just do nothing. This is relatively safe to do.

However, it would be better if the page had a way to query what elements are available, so it can adjust its content to be appropriate.
Priority: -- → P3
Attached patch Add UITour.getAvailableTargets (obsolete) — Splinter Review
This seems to work.  It's a little tricky mapping widget IDs/names to targets.
Attachment #8372717 - Flags: review?(bmcbride)
Comment on attachment 8372717 [details] [diff] [review]
Add UITour.getAvailableTargets

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

::: browser/modules/UITour.jsm
@@ +256,5 @@
>          this.getConfiguration(contentDocument, data.configuration, data.callbackID);
>          break;
>        }
> +
> +      case "getAvailableTargets": {

Drive-by comment: I was thinking we would reuse getConfiguration for this

@@ +836,5 @@
>      }
>      this.sendPageCallback(aContentDocument, aCallbackId, config);
>    },
> +
> +  getAvailableTargets: function(aContentDocument, aCallbackId) {

I think this is complex enough to deserve tests.
Thanks, Matt.  I'd like to get an r+ or at least a feedback+ from Blair (or anybody else I guess who's familiar with this bug) that the patch is doing the right thing before working on tests.
Comment on attachment 8372717 [details] [diff] [review]
Add UITour.getAvailableTargets

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

Hm, this approach doesn't handle targets that aren't widgets managed by CustomizableUI, such as "quit" and "customize". It also doesn't take into account widgets that aren't available in all windows - such as those not available in private-browsing windows.

Seems easiest/most reliable to just kick off a bunch of parallel getTarget() calls. And either way, we should cache the results per-window (the clear the cache any time we see a customization event).

Also, we should advertise the availability of "pinnedTab", which is a target we dynamically create (so that shouldn't go through getTarget).

::: browser/modules/UITour.jsm
@@ +256,5 @@
>          this.getConfiguration(contentDocument, data.configuration, data.callbackID);
>          break;
>        }
> +
> +      case "getAvailableTargets": {

(In reply to Matthew N. [:MattN] from comment #2)
> Drive-by comment: I was thinking we would reuse getConfiguration for this

What Matt said.

@@ +836,5 @@
>      }
>      this.sendPageCallback(aContentDocument, aCallbackId, config);
>    },
> +
> +  getAvailableTargets: function(aContentDocument, aCallbackId) {

aCallbackID - or someone's going to end up screaming profanities sometime in the future because of us using Id and ID in the same module.
Attachment #8372717 - Flags: review?(bmcbride) → review-
DOOMED
Assignee: nobody → adw
Status: NEW → ASSIGNED
I don't think we're going to need this for the Australis update tour because most things will be in a default position for Australis. Holly, is that fine that this doesn't make it for 29?
Flags: needinfo?(hhabstritt.bugzilla)
Whiteboard: [Australis:M-]
I see, thanks.

(In reply to Blair McBride [:Unfocused] from comment #4)
> @@ +836,5 @@
> >      }
> >      this.sendPageCallback(aContentDocument, aCallbackId, config);
> >    },
> > +
> > +  getAvailableTargets: function(aContentDocument, aCallbackId) {
> 
> aCallbackID - or someone's going to end up screaming profanities sometime in
> the future because of us using Id and ID in the same module.

But this module uses both!  Look a couple of lines above the two added lines I just quoted (and you quoted). :-)  ID seems to be more popular, so I did change it.
Attachment #8372717 - Attachment is obsolete: true
Attachment #8384086 - Flags: review?(bmcbride)
Comment on attachment 8384086 [details] [diff] [review]
add UITour.getConfiguration("availableTargets")

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

Most excellent.

::: browser/modules/UITour.jsm
@@ +935,5 @@
>      });
>      aWindow.gBrowser.selectedTab = tab;
>    },
>  
>    getConfiguration: function(aContentDocument, aConfiguration, aCallbackId) {

*sigh* Don't suppose you'd like to just rename this to aCallbackID while you're at it? :)

::: browser/modules/test/browser_UITour_availableTargets.js
@@ +67,5 @@
> +];
> +
> +function ok_targets(actualData, expectedTargets) {
> +  // Depending on how soon after page load this is called, the selected tab icon
> +  // may or may not be showing the loading throbber.  Check for its presence and

Ugh, we should fix that. File a bug?
Attachment #8384086 - Flags: review?(bmcbride) → review+
(In reply to Matthew N. [:MattN] from comment #6)
> I don't think we're going to need this for the Australis update tour because
> most things will be in a default position for Australis. Holly, is that fine
> that this doesn't make it for 29?

I don't consider this a blocker. But saying that, it does allow fixing breakage in the tour, so I would like it uplifted to Aurora. I don't think it comes with much risk in doing so - especially given we can just opt to not use it if it ends up breaking.
https://hg.mozilla.org/integration/fx-team/rev/23b6dc0350c8

(In reply to Blair McBride [:Unfocused] from comment #8)
> >    getConfiguration: function(aContentDocument, aConfiguration, aCallbackId) {
> 
> *sigh* Don't suppose you'd like to just rename this to aCallbackID while
> you're at it? :)

Fixed, and fixed the couple of other instances, involving "aId".

> > +  // Depending on how soon after page load this is called, the selected tab icon
> > +  // may or may not be showing the loading throbber.  Check for its presence and
> 
> Ugh, we should fix that. File a bug?

bug 979103
https://hg.mozilla.org/mozilla-central/rev/23b6dc0350c8
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
(In reply to Blair McBride [:Unfocused] from comment #9)
> I don't consider this a blocker. But saying that, it does allow fixing
> breakage in the tour, so I would like it uplifted to Aurora. I don't think
> it comes with much risk in doing so - especially given we can just opt to
> not use it if it ends up breaking.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): It's a new feature.
User impact if declined: Parts of Australis UITour content pages may be broken until Firefox 30.
Testing completed (on m-c, etc.): browser-chrome test on m-c and fx-team.
Risk to taking this patch (and alternatives if risky): Small.
String or IDL/UUID changes made by this patch: None.
Attachment #8385812 - Flags: approval-mozilla-aurora?
Attachment #8385812 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Can we please update australis-uitour.js to reflect the change here? Thanks
No, Unfocused is: https://github.com/Unfocused/mozilla-uitour

I said I would do it but didn't have time on Friday and I'm on PTO today and tomorrow so if someone else wants to that would be good.
Oh.  Alex, if it's OK that your repo doesn't match Blair's, you can just replace your getSyncConfiguration with the getConfiguration from this test: http://mxr.mozilla.org/mozilla-central/source/browser/modules/test/uitour.js#168

Then call it with "availableTargets" as the configName.
And "sync" for the old getSyncConfiguration behavior.
Flags: needinfo?(hhabstritt.bugzilla)
Thanks, Blair

One question about this API, does it only return a list of available targets, or can it also tell me where in the chrome a target is located? I seem to remember a discussion that the latter would be useful, but I wasn't sure if this bug covers that.
Depends on: 988151
(In reply to Alex Gibson [:agibson] from comment #20)
> One question about this API, does it only return a list of available
> targets, or can it also tell me where in the chrome a target is located? I
> seem to remember a discussion that the latter would be useful, but I wasn't
> sure if this bug covers that.

It's just an array of targets that aren't in the palette. I filed bug 988151 to get more visibility/location information returned.
Depends on: 988305
Keywords: verifyme
Is there a way to verify this manually?

I removed Add-ons and Bookmarks  button from Menu Panel and Toolbar but from an user point of view nothing was changed postfix. The pages from the Ui Tour related to Add-ons and Bookmarks are still shown whether the buttons are visible or not.

Builds used for testing:
Prefix: 
Aurora 29
Build Id: 20140303004002

Postfix:
Beta 29 RC candidate
Build Id:20140421221237
(In reply to Catalin Varga [QA][:VarCat] from comment #22)
> Is there a way to verify this manually?
> 
> I removed Add-ons and Bookmarks  button from Menu Panel and Toolbar but from
> an user point of view nothing was changed postfix. The pages from the Ui
> Tour related to Add-ons and Bookmarks are still shown whether the buttons
> are visible or not.

Note: this API is currently not used in the tour itself, so it will need testing separately.
No human verification is needed at this time.
Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.