Adopt template strings in UnifiedComplete

RESOLVED FIXED in mozilla34

Status

()

Toolkit
Places
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mano, Assigned: Michael Pruett, Mentored)

Tracking

unspecified
mozilla34
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

16.09 KB, patch
mano
: review+
Details | Diff | Splinter Review
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@mozilla.com
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.

Updated

3 years ago
Blocks: 995091

Updated

3 years ago
Whiteboard: [good-first-bug][lang=js] → [good first bug][lang=js]
(Assignee)

Comment 6

3 years ago
Created attachment 8460881 [details] [diff] [review]
Patch

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.

Comment 8

3 years ago
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+
(Assignee)

Comment 10

3 years ago
Created attachment 8468531 [details] [diff] [review]
Patch

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)
Attachment #8468531 - Flags: review?(mano) → review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/827276e298e8
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/827276e298e8
Status: NEW → RESOLVED
Last Resolved: 3 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.