Closed Bug 1041429 Opened 6 years ago Closed 6 years ago

Adopt template strings in UnifiedComplete

Categories

(Toolkit :: Places, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: mano, Assigned: michael, Mentored)

References

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 file, 1 obsolete file)

Now that template strings are implemented, all that string.replace mess in UnifiedComplete may be cleaned up nicely.
also in _processRow and probably in few other places
Yeah, let's do this. Can you mentor this? It seems like a good first or second bug, no?
Flags: needinfo?(mano)
OS: Mac OS X → All
Hardware: x86 → All
Sure!
Flags: needinfo?(mano)
Whiteboard: [mentor=mano]
We actually have a separate bugzilla field for that now :)
Mentor: mano
Whiteboard: [mentor=mano] → [good-first-bug][lang=js]
Nice.

*** 

function sql(...parts) parts.join(" ") can also be removed in favor of "no substitution template" string usage.
Whiteboard: [good-first-bug][lang=js] → [good first bug][lang=js]
Attached patch Patch (obsolete) — Splinter Review
I've updated UnifiedComplete.js to use template strings in place of String.replace() where appropriate. I've also replaced uses of the sql() function with multi-line strings.

https://tbpl.mozilla.org/?tree=Try&rev=b65757c0edc3
Attachment #8460881 - Flags: review?(mano)
Template strings are currently #ifdef NIGHTLY_BUILD, it's probably a good idea to put any consumers behind that as well to avoid surprises at Aurora uplift time.
Bug 1038259 removes the flag. It's not landed yet, but won't be too long.
Comment on attachment 8460881 [details] [diff] [review]
Patch

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

Unfortunately the patch doesn't apply anymore (my fault for not reviewing it promptly). Given the nature of this patch (most of the changes are in a single, quite large hunk), you may want to back out the "breaking" patches locally, reapply your patch, reapply the other patches, find out if there are more places which need update (any new sql/replace usage), and then generate the new patch. Please re-run the tests. Autocomplete code tends to break easily.

Sorry for that :(

::: toolkit/components/places/UnifiedComplete.js
@@ +100,5 @@
>  // TODO bug 412736: in case of a frecency tie, we might break it with h.typed
>  // and h.visit_count.  That is slower though, so not doing it yet...
> +function defaultQuery(conditions) {
> +  if (conditions === undefined)
> +    conditions = "";

Use the default argument syntax.

function defaultQuery(conditions = "") {

@@ +189,4 @@
>  
> +function hostQuery(conditions) {
> +  if (conditions === undefined)
> +    conditions = "";

Ditto.

@@ +207,5 @@
> +const SQL_TYPED_HOST_QUERY = hostQuery("AND typed = 1");
> +
> +function urlQuery(conditions) {
> +  if (conditions === undefined)
> +    conditions = "";

Same Same. Please double check if I overlooked any instance of that.
Attachment #8460881 - Flags: review?(mano) → review+
Attached patch PatchSplinter Review
Thanks, I've updated the patch to the most recent version of UnifiedComplete.js. I've updated the query functions to use the default argument syntax as you suggested.

Here are the results from the try server:
https://tbpl.mozilla.org/?tree=Try&rev=664510e6aa77
Assignee: nobody → michael
Attachment #8460881 - Attachment is obsolete: true
Attachment #8468531 - Flags: review?(mano)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/827276e298e8
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
QA Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.