Improve infrastructure dealing with moz-action: autocomplete results

VERIFIED FIXED in Firefox 35

Status

()

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: Unfocused, Assigned: Unfocused)

Tracking

(Blocks 1 bug)

unspecified
mozilla35
Points:
8
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(firefox35 verified)

Details

Attachments

(1 attachment)

The infrastructure for handling moz-action: autocomplete results is kinda half baked. When I originally added this (for switch-to-tab), it was intentionally left in a state where it worked, but wasn't fully completed - because at the time it wasn't needed. But now we do need it, for the likes of bug 951624. So it's time to fix this up finally.

Which means:
* Adding proper support for other action types
* Adding support for more complex actions
* Cleaning up the code that handles the UI for autocomplete results, so it scales better with more result types added
Flags: qe-verify-
Flags: firefox-backlog+
Marc: This is a spin-off from bug 951624.
Flags: needinfo?(mmucci)
Posted patch Patch v1Splinter Review
Attachment #8487064 - Flags: review?(mak77)
Added to IT 35.1

(In reply to Blair McBride [:Unfocused] from comment #1)
> Marc: This is a spin-off from bug 951624.
Iteration: --- → 35.1
Flags: needinfo?(mmucci)
Comment on attachment 8487064 [details] [diff] [review]
Patch v1

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

::: browser/base/content/urlbarBindings.xml
@@ +728,5 @@
> +          let [, action, params] = aUrl.match(/^moz-action:([^,]+),(.*)$/);
> +
> +          return {
> +                   type: action,
> +                   params: JSON.parse(params)

I wonder if we should have a fallback here (for now) that would allow an old switchtotab uri to still be parsed properly... if JSON.parse throws we might look if params is an url and build the object manually.

This would still allow us to disable unified, temporarily, without losing switchtotab support.

::: toolkit/components/places/UnifiedComplete.js
@@ +498,5 @@
> + * @param action
> + *        Name of the action
> + * @param params
> + *        Object, whose keys are paramter names and values or the corresponding
> + *        paramter values.

I'm not totally sure I understand what this means... and typo: paramter
maybe you meant s/or/are/?

@@ +503,5 @@
> + * @return String representation of the built moz-action: URL
> + */
> +function makeActionURL(action, params) {
> +  let url = "moz-action:" + action + "," + JSON.stringify(params);
> +  // Make a nsIURI out of this to ensure it's encoeded properly.

typo: encoeded

::: toolkit/content/widgets/autocomplete.xml
@@ +1476,5 @@
>            }
>  
>            // Check if we have a search engine name
> +          if (types.has("search")) {
> +            emphasiseUrl = false;

where is emphasiseUrl declared? I can't find it on mxr or locally

@@ +1543,5 @@
>            if (title == "")
>              title = url;
>  
>            // Emphasize the matching search terms for the description
> +          this._setUpDescription(this._title, title, !emphasiseTitle);

as well as I can't find emphasiseTitle anywhere... is this working just cause it's undefined?

@@ +1567,5 @@
> +          let [, action, params] = aUrl.match(/^moz-action:([^,]+),(.*)$/);
> +
> +          return {
> +                   type: action,
> +                   params: JSON.parse(params)

ditto!
Attachment #8487064 - Flags: review?(mak77) → review+
(In reply to Marco Bonardo [:mak] (needinfo? me) from comment #4)
> I wonder if we should have a fallback here (for now) that would allow an old
> switchtotab uri to still be parsed properly... if JSON.parse throws we might
> look if params is an url and build the object manually.
> 
> This would still allow us to disable unified, temporarily, without losing
> switchtotab support.

Oh! Nice idea :) Done.

> I'm not totally sure I understand what this means... and typo: paramter
> maybe you meant s/or/are/?

We need linting for grammar and spelling in comments :\

> where is emphasiseUrl declared? I can't find it on mxr or locally

Whoops. Hiding somewhere in another patch.
Er, actually, this should get some verification - just checking that nothing has changed.
Flags: qe-verify- → qe-verify+
sorry had to backout this change for test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=47873145&tree=Fx-Team
Note to self for tomorrow: Forgot to update browser_autocomplete_a11y_label.js to use the new moz-action: JSON encoded format.

Or, in other words, I'm bitrotting myself again.

Updated

5 years ago
QA Contact: andrei.vaida
Iteration: 35.1 → 35.2
https://hg.mozilla.org/mozilla-central/rev/c6ada055be38
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Per my conversation with :Unfocused, I've performed an overall smoke on the Awesomebar since there wasn't a specific thing for manual QA to poke at. Testing was done on Nightly 35.0a1 (2014-09-16), using Windows 7 64-bit, Ubuntu 14.04 LTS 32-bit and Mac OS X 10.7.5.
Status: RESOLVED → VERIFIED
Depends on: 1071986
You need to log in before you can comment on or make changes to this bug.